Hi,
The attached patch adds an initial gpo-smb implementation. It uses SMB to retrieve HBAC policy files, evaluate them, and enforce the whitelists and blacklists found in those policy files. This is an initial implementation and there are a few things that I am planning on implementing, but are not part of this patch.
This patch does not implement: * the "permissive" mode, in which the policy files are evaluated, but not enforced * a separate gpo_child process to make the blocking libsmbclient calls * use of a cache * few other things
As such, most of this patch will be moved to a separate gpo_child process in a subsequent patch.
Also note that some (but not all) of the changes that were pushed to master as part of my patch titled "add libsmbclient to makefiles" seem to have been deleted by subsequent pushes to master. As such, this patch reintroduces those changes (which only affect configure.ac and libsmbclient.m4).
Regards, Yassir.
On Thu, May 29, 2014 at 04:09:21AM -0400, Yassir Elley wrote:
Also note that some (but not all) of the changes that were pushed to master as part of my patch titled "add libsmbclient to makefiles" seem to have been deleted by subsequent pushes to master. As such, this patch reintroduces those changes (which only affect configure.ac and libsmbclient.m4).
That would be worrisome, what changes in particular? I'm not aware of pushing anything else than acked patches from the list..
Are you talking about the autoconf refactor Lukas did? IIRC that one only merged some code duplicates into a single file..
----- Original Message -----
On Thu, May 29, 2014 at 04:09:21AM -0400, Yassir Elley wrote:
Also note that some (but not all) of the changes that were pushed to master as part of my patch titled "add libsmbclient to makefiles" seem to have been deleted by subsequent pushes to master. As such, this patch reintroduces those changes (which only affect configure.ac and libsmbclient.m4).
That would be worrisome, what changes in particular? I'm not aware of pushing anything else than acked patches from the list..
Are you talking about the autoconf refactor Lukas did? IIRC that one only merged some code duplicates into a single file.. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Hi Jakub,
You are right. I overlooked the fact that the autoconf refactor had moved things around. My mistake.
Revised patch attached.
Regards, Yassir.
On (29/05/14 09:06), Yassir Elley wrote:
----- Original Message -----
On Thu, May 29, 2014 at 04:09:21AM -0400, Yassir Elley wrote:
Also note that some (but not all) of the changes that were pushed to master as part of my patch titled "add libsmbclient to makefiles" seem to have been deleted by subsequent pushes to master. As such, this patch reintroduces those changes (which only affect configure.ac and libsmbclient.m4).
That would be worrisome, what changes in particular? I'm not aware of pushing anything else than acked patches from the list..
Are you talking about the autoconf refactor Lukas did? IIRC that one only merged some code duplicates into a single file.. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Hi Jakub,
You are right. I overlooked the fact that the autoconf refactor had moved things around. My mistake.
Revised patch attached.
Regards, Yassir.
From abc4ac3e664ccbeeb58d131dc6c37cc314f5ba3d Mon Sep 17 00:00:00 2001 From: Yassir Elley yelley@redhat.com Date: Wed, 21 May 2014 15:11:36 -0400 Subject: [PATCH] AD-GPO: Add initial gpo-smb implementation
src/providers/ad/ad_gpo.c | 680 +++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 676 insertions(+), 4 deletions(-)
diff --git a/src/providers/ad/ad_gpo.c b/src/providers/ad/ad_gpo.c index 168ed2d7e8657799a7c773ec8b24897cdd303ec4..519c0dbec4d85e7f6e41db672ae0f2c2f720d81b 100644 --- a/src/providers/ad/ad_gpo.c +++ b/src/providers/ad/ad_gpo.c @@ -31,6 +31,9 @@ */
#include <security/pam_modules.h> +#include <libsmbclient.h> +#include <ini_configobj.h> +#include <ini_config.h>
^^^^^^^^^^^^ This is the old ini_config interface. You can remove this line.
#include "util/util.h" #include "util/strtonum.h" #include "providers/data_provider.h"
// snip
- int ret;
- char **allow_sids = NULL; char **deny_sids = NULL;
- int allow_size = 0; int deny_size = 0;
- ret = ini_config_create(&ini_config);
- if (ret) goto done;
- ret = ini_config_file_from_mem(data_buf, data_len, &file_ctx);
^^^^^^^^^^^^^^^^^^^^^^^^ This function is available in libini_config-1.1 Please, update buildrequires in spec file.
diff --git a/contrib/sssd.spec.in b/contrib/sssd.spec.in index b5e7903..10704ae 100644 --- a/contrib/sssd.spec.in +++ b/contrib/sssd.spec.in @@ -97,7 +97,7 @@ BuildRequires: libtdb-devel BuildRequires: libldb-devel BuildRequires: libdhash-devel >= 0.4.2 BuildRequires: libcollection-devel -BuildRequires: libini_config-devel +BuildRequires: libini_config-devel >= 1.1 BuildRequires: dbus-devel BuildRequires: dbus-libs %if 0%{?rhel5_minor} >= 7
LS
----- Original Message -----
On (29/05/14 09:06), Yassir Elley wrote:
----- Original Message -----
On Thu, May 29, 2014 at 04:09:21AM -0400, Yassir Elley wrote:
Also note that some (but not all) of the changes that were pushed to master as part of my patch titled "add libsmbclient to makefiles" seem to have been deleted by subsequent pushes to master. As such, this patch reintroduces those changes (which only affect configure.ac and libsmbclient.m4).
That would be worrisome, what changes in particular? I'm not aware of pushing anything else than acked patches from the list..
Are you talking about the autoconf refactor Lukas did? IIRC that one only merged some code duplicates into a single file.. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Hi Jakub,
You are right. I overlooked the fact that the autoconf refactor had moved things around. My mistake.
Revised patch attached.
Regards, Yassir.
From abc4ac3e664ccbeeb58d131dc6c37cc314f5ba3d Mon Sep 17 00:00:00 2001 From: Yassir Elley yelley@redhat.com Date: Wed, 21 May 2014 15:11:36 -0400 Subject: [PATCH] AD-GPO: Add initial gpo-smb implementation
src/providers/ad/ad_gpo.c | 680 +++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 676 insertions(+), 4 deletions(-)
diff --git a/src/providers/ad/ad_gpo.c b/src/providers/ad/ad_gpo.c index 168ed2d7e8657799a7c773ec8b24897cdd303ec4..519c0dbec4d85e7f6e41db672ae0f2c2f720d81b 100644 --- a/src/providers/ad/ad_gpo.c +++ b/src/providers/ad/ad_gpo.c @@ -31,6 +31,9 @@ */
#include <security/pam_modules.h> +#include <libsmbclient.h> +#include <ini_configobj.h> +#include <ini_config.h>
^^^^^^^^^^^^
This is the old ini_config interface. You can remove this line.
#include "util/util.h" #include "util/strtonum.h" #include "providers/data_provider.h"
// snip
- int ret;
- char **allow_sids = NULL; char **deny_sids = NULL;
- int allow_size = 0; int deny_size = 0;
- ret = ini_config_create(&ini_config);
- if (ret) goto done;
- ret = ini_config_file_from_mem(data_buf, data_len, &file_ctx);
^^^^^^^^^^^^^^^^^^^^^^^^
This function is available in libini_config-1.1 Please, update buildrequires in spec file.
diff --git a/contrib/sssd.spec.in b/contrib/sssd.spec.in index b5e7903..10704ae 100644 --- a/contrib/sssd.spec.in +++ b/contrib/sssd.spec.in @@ -97,7 +97,7 @@ BuildRequires: libtdb-devel BuildRequires: libldb-devel BuildRequires: libdhash-devel >= 0.4.2 BuildRequires: libcollection-devel -BuildRequires: libini_config-devel +BuildRequires: libini_config-devel >= 1.1 BuildRequires: dbus-devel BuildRequires: dbus-libs %if 0%{?rhel5_minor} >= 7
LS
Revised patch attached.
Also, I successfully tested the code after installing the ding-libs packages from lslebodn's repo. Previously, I was using rpms that I had manually compiled (for the ini_config_file_from_mem functionality).
Thanks, Yassir.
On Fri, May 30, 2014 at 12:35:32PM -0400, Yassir Elley wrote:
----- Original Message -----
On (29/05/14 09:06), Yassir Elley wrote:
----- Original Message -----
On Thu, May 29, 2014 at 04:09:21AM -0400, Yassir Elley wrote:
Also note that some (but not all) of the changes that were pushed to master as part of my patch titled "add libsmbclient to makefiles" seem to have been deleted by subsequent pushes to master. As such, this patch reintroduces those changes (which only affect configure.ac and libsmbclient.m4).
That would be worrisome, what changes in particular? I'm not aware of pushing anything else than acked patches from the list..
Are you talking about the autoconf refactor Lukas did? IIRC that one only merged some code duplicates into a single file.. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Hi Jakub,
You are right. I overlooked the fact that the autoconf refactor had moved things around. My mistake.
Revised patch attached.
Regards, Yassir.
From abc4ac3e664ccbeeb58d131dc6c37cc314f5ba3d Mon Sep 17 00:00:00 2001 From: Yassir Elley yelley@redhat.com Date: Wed, 21 May 2014 15:11:36 -0400 Subject: [PATCH] AD-GPO: Add initial gpo-smb implementation
src/providers/ad/ad_gpo.c | 680 +++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 676 insertions(+), 4 deletions(-)
diff --git a/src/providers/ad/ad_gpo.c b/src/providers/ad/ad_gpo.c index 168ed2d7e8657799a7c773ec8b24897cdd303ec4..519c0dbec4d85e7f6e41db672ae0f2c2f720d81b 100644 --- a/src/providers/ad/ad_gpo.c +++ b/src/providers/ad/ad_gpo.c @@ -31,6 +31,9 @@ */
#include <security/pam_modules.h> +#include <libsmbclient.h> +#include <ini_configobj.h> +#include <ini_config.h>
^^^^^^^^^^^^
This is the old ini_config interface. You can remove this line.
#include "util/util.h" #include "util/strtonum.h" #include "providers/data_provider.h"
// snip
- int ret;
- char **allow_sids = NULL; char **deny_sids = NULL;
- int allow_size = 0; int deny_size = 0;
- ret = ini_config_create(&ini_config);
- if (ret) goto done;
- ret = ini_config_file_from_mem(data_buf, data_len, &file_ctx);
^^^^^^^^^^^^^^^^^^^^^^^^
This function is available in libini_config-1.1 Please, update buildrequires in spec file.
diff --git a/contrib/sssd.spec.in b/contrib/sssd.spec.in index b5e7903..10704ae 100644 --- a/contrib/sssd.spec.in +++ b/contrib/sssd.spec.in @@ -97,7 +97,7 @@ BuildRequires: libtdb-devel BuildRequires: libldb-devel BuildRequires: libdhash-devel >= 0.4.2 BuildRequires: libcollection-devel -BuildRequires: libini_config-devel +BuildRequires: libini_config-devel >= 1.1 BuildRequires: dbus-devel BuildRequires: dbus-libs %if 0%{?rhel5_minor} >= 7
LS
Revised patch attached.
Also, I successfully tested the code after installing the ding-libs packages from lslebodn's repo. Previously, I was using rpms that I had manually compiled (for the ini_config_file_from_mem functionality).
Thanks, Yassir.
I didn't had a chance to do functional testings yet, I'll send my results tomorrow. For a start please find my comments on the code inline.
bye, Sumit
From 24625d6cd02faeb39e8522769d470b3a8beed513 Mon Sep 17 00:00:00 2001 From: Yassir Elley yelley@redhat.com Date: Wed, 21 May 2014 15:11:36 -0400 Subject: [PATCH] AD-GPO: Add initial gpo-smb implementation
contrib/sssd.spec.in | 2 +- src/providers/ad/ad_gpo.c | 679 +++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 676 insertions(+), 5 deletions(-)
diff --git a/contrib/sssd.spec.in b/contrib/sssd.spec.in index b5e7903713ccc6944fbed4f84c2f0413a257b760..10704ae48cf5636de2a5cbbb42fc00b99052144f 100644 --- a/contrib/sssd.spec.in +++ b/contrib/sssd.spec.in @@ -97,7 +97,7 @@ BuildRequires: libtdb-devel BuildRequires: libldb-devel BuildRequires: libdhash-devel >= 0.4.2 BuildRequires: libcollection-devel -BuildRequires: libini_config-devel +BuildRequires: libini_config-devel >= 1.1
Please add this new minimal version to the configure check as well.
BuildRequires: dbus-devel BuildRequires: dbus-libs %if 0%{?rhel5_minor} >= 7 diff --git a/src/providers/ad/ad_gpo.c b/src/providers/ad/ad_gpo.c index bb6896e70ae96036b1a9e58a6c8e25d3d8f1543f..9a0f09f9d2735158d970bb19d631645eb8ae9cfa 100644 --- a/src/providers/ad/ad_gpo.c +++ b/src/providers/ad/ad_gpo.c
...
@@ -543,6 +571,235 @@ ad_gpo_filter_gpos_by_dacl(TALLOC_CTX *mem_ctx, return ret; }
+/*
- This function determines whether the input cse_guid matches any of the input
- gpo_cse_guids. The boolean result is assigned to the _included output param.
- */
+static errno_t +ad_gpo_includes_cse_guid(const char *cse_guid,
const char **gpo_cse_guids,
int num_gpo_cse_guids,
bool *_included)
In general I prefer that a function returns an error code and results are returned by reference. But in this case no error is returned only EOK and I think it would be easier to just return the bool value directly. If you prefer to keep the error code, e.g. because you plan to add some NULL checks for cse_guid and gpo_cse_guids and return EINVAL in this case, please check the return value when calling ad_gpo_includes_cse_guid() below.
+{
- int i = 0;
- const char *gpo_cse_guid = NULL;
- for (i = 0; i < num_gpo_cse_guids; i++) {
gpo_cse_guid = gpo_cse_guids[i];
if (strcmp(gpo_cse_guid, cse_guid) == 0) {
*_included = true;
return EOK;
}
- }
- *_included = false;
- return EOK;
+}
+/*
- This function takes an input dacl_filtered_gpos list, filters out any gpo
- that does not contain the input cse_guid, and assigns the result to the
- _cse_filtered_gpos output parameter.
- */
+static errno_t +ad_gpo_filter_gpos_by_cse_guid(TALLOC_CTX *mem_ctx,
const char *cse_guid,
struct gp_gpo **dacl_filtered_gpos,
int num_dacl_filtered_gpos,
struct gp_gpo ***_cse_filtered_gpos,
int *_num_cse_filtered_gpos)
+{
- TALLOC_CTX *tmp_ctx = NULL;
- int i = 0;
- int ret = 0;
- struct gp_gpo *dacl_filtered_gpo = NULL;
- int gpo_dn_idx = 0;
- struct gp_gpo **cse_filtered_gpos = NULL;
- bool included;
- tmp_ctx = talloc_new(NULL);
- if (tmp_ctx == NULL) {
ret = ENOMEM;
goto done;
- }
- cse_filtered_gpos = talloc_array(tmp_ctx,
struct gp_gpo *,
num_dacl_filtered_gpos + 1);
- if (cse_filtered_gpos == NULL) {
ret = ENOMEM;
goto done;
- }
- for (i = 0; i < num_dacl_filtered_gpos; i++) {
dacl_filtered_gpo = dacl_filtered_gpos[i];
DEBUG(SSSDBG_TRACE_ALL, "examining cse candidate_gpo_guid: %s\n",
dacl_filtered_gpo->gpo_guid);
ret = ad_gpo_includes_cse_guid(cse_guid,
dacl_filtered_gpo->gpo_cse_guids,
dacl_filtered_gpo->num_gpo_cse_guids,
&included);
see above
if (included) {
DEBUG(SSSDBG_TRACE_ALL,
"GPO applicable to target per cse_guid filtering\n");
cse_filtered_gpos[gpo_dn_idx] = talloc_steal(cse_filtered_gpos,
dacl_filtered_gpo);
Do you need the item still in dacl_filtered_gpos? If not I would prefer to set dacl_filtered_gpos[i] to NULL or call talloc_move on it. If it is still needed in dacl_filtered_gpos I wonder if it would be safer to copy the item to avoid that someone iterates over the dacl_filtered_gpos elements and frees them individually?
gpo_dn_idx++;
} else {
DEBUG(SSSDBG_TRACE_ALL,
"GPO not applicable to target per cse_guid filtering\n");
continue;
}
- }
- cse_filtered_gpos[gpo_dn_idx] = NULL;
- *_cse_filtered_gpos = talloc_steal(mem_ctx, cse_filtered_gpos);
- *_num_cse_filtered_gpos = gpo_dn_idx;
- ret = EOK;
- done:
- talloc_free(tmp_ctx);
- return ret;
+}
+/*
- This cse-specific function (GP_EXT_GUID_SECURITY) populates the output
- parameter (found) based on whether the input user_sid or any of the input
- group_sids appear in the input list of privilege_sids.
- */
+static void +check_rights(char **privilege_sids,
int privilege_size,
const char *user_sid,
const char **group_sids,
int group_size,
int *found)
I would suggest to return 'found' directly here as well and maybe convert it to a bool or is there a reason for having an int here?
+{
- int match = 0;
- int i, j;
- for (i = 0; i < privilege_size; i++) {
- if (strcmp(user_sid, privilege_sids[i]) == 0) {
match = 1;
break;
- }
- for (j = 0; j < group_size; j++) {
if (strcmp(group_sids[j], privilege_sids[i]) == 0) {
- match = 1;
- break;
^ Tab indent
}
- }
- }
- *found = match;
+}
+/*
- This cse-specific function (GP_EXT_GUID_SECURITY) performs HBAC policy
- application and determines whether logon access is granted or denied for
- the {user,domain} tuple specified in the inputs. This function returns EOK
- to indicate that access is granted. Any other return value indicates that
- access is denied.
- The access control algorithm first determines whether the "principal_sids"
- (i.e. user_sid or group_sids) appear in allowed_sids and denied_sids.
- For access to be granted, both the "allowed_sids_condition" *and* the
- "denied_sids_condition" must be met (in all other cases, access is denied).
- The "allowed_sids_condition" is satisfied if any of the principal_sids
- appears in allowed_sids OR if the allowed_sids list is empty
- The "denied_sids_condition" is satisfied if none of the principal_sids
- appear in denied_sids
- Note that a deployment that is unaware of GPO-based access-control policy
- settings is unaffected by them (b/c the absence of allowed_sids grants access).
- Note that if a principal_sid appears in both allowed_sids and denied_sids,
- the "allowed_sids_condition" is met, but the "denied_sids_condition" is not.
- In other words, Deny takes precedence over Allow.
I have two comments here. First, is there a way to avoid having deny rules? The reason is that we had some troubles in the past here, because when checking deny rules all group memberships of a user must be available to properly deny access. If some are missing access might be granted although it should be rejected. I guess it is not possible to get rid of them because they will be set on the AD side and we have to respect them.
Second, you are only checking SeInteractiveLogonRight and SeDenyInteractiveLogonRight. What about Se[Deny][Network|Batch|Service|RemoteInteractive]LogonRight? Are you planning to integrate them as well and relate them to different PAM services?
- */
+static errno_t +ad_gpo_access_check(TALLOC_CTX *mem_ctx,
const char *user,
struct sss_domain_info *domain,
char **allowed_sids,
int allowed_size,
char **denied_sids,
int denied_size)
+{
- const char *user_sid;
- const char **group_sids;
- int group_size = 0;
- int access_granted = 0;
- int access_denied = 0;
Is there a reason to use int instead of bool?
- int ret;
- int j;
- DEBUG(SSSDBG_TRACE_FUNC, "POLICY FILE:\n");
- DEBUG(SSSDBG_TRACE_FUNC, "allowed_size = %d\n", allowed_size);
- for (j= 0; j < allowed_size; j++) {
DEBUG(SSSDBG_TRACE_FUNC, "allowed_sids[%d] = %s\n", j,
allowed_sids[j]);
- }
- DEBUG(SSSDBG_TRACE_FUNC, "denied_size = %d\n", denied_size);
- for (j= 0; j < denied_size; j++) {
DEBUG(SSSDBG_TRACE_FUNC, " denied_sids[%d] = %s\n", j,
denied_sids[j]);
- }
- ret = ad_gpo_get_sids(mem_ctx, user, domain, &user_sid,
&group_sids, &group_size);
- if (ret != EOK) {
ret = ERR_NO_SIDS;
DEBUG(SSSDBG_OP_FAILURE,
"Unable to retrieve SIDs: [%d](%s)\n", ret, sss_strerror(ret));
goto done;
- }
- DEBUG(SSSDBG_TRACE_FUNC, "CURRENT USER:\n");
- DEBUG(SSSDBG_TRACE_FUNC, " user_sid = %s\n", user_sid);
- for (j= 0; j < group_size; j++) {
DEBUG(SSSDBG_TRACE_FUNC, " group_sids[%d] = %s\n", j,
group_sids[j]);
- }
- /* If AllowLogonLocally is not defined, all users are allowed */
- if (allowed_size == 0) {
access_granted = 1;
- } else {
check_rights(allowed_sids, allowed_size, user_sid,
group_sids, group_size, &access_granted);
- }
- DEBUG(SSSDBG_TRACE_FUNC, " access_granted = %d\n", access_granted);
- check_rights(denied_sids, denied_size, user_sid,
group_sids, group_size, &access_denied);
- DEBUG(SSSDBG_TRACE_FUNC, " access_denied = %d\n", access_denied);
- if (access_granted && !access_denied) {
return EOK;
- } else {
return EACCES;
- }
- done:
- if (ret) {
DEBUG(SSSDBG_CRIT_FAILURE, "Error encountered: %d.\n", ret);
- }
- return ret;
+}
/* == ad_gpo_access_send/recv implementation ================================*/
struct ad_gpo_access_state { @@ -558,6 +815,9 @@ struct ad_gpo_access_state { const char *target_dn; struct gp_gpo **dacl_filtered_gpos; int num_dacl_filtered_gpos;
- struct gp_gpo **cse_filtered_gpos;
- int num_cse_filtered_gpos;
- int cse_gpo_index;
};
static void ad_gpo_connect_done(struct tevent_req *subreq); @@ -565,6 +825,9 @@ static void ad_gpo_target_dn_retrieval_done(struct tevent_req *subreq); static void ad_gpo_process_som_done(struct tevent_req *subreq); static void ad_gpo_process_gpo_done(struct tevent_req *subreq);
+static errno_t ad_gpo_cse_step(struct tevent_req *req); +static void ad_gpo_cse_done(struct tevent_req *subreq);
struct tevent_req * ad_gpo_access_send(TALLOC_CTX *mem_ctx, struct tevent_context *ev, @@ -586,6 +849,9 @@ ad_gpo_access_send(TALLOC_CTX *mem_ctx, state->domain = domain; state->dacl_filtered_gpos = NULL; state->num_dacl_filtered_gpos = 0;
- state->cse_filtered_gpos = NULL;
- state->num_cse_filtered_gpos = 0;
- state->cse_gpo_index = -1; state->ev = ev; state->user = user; state->ldb_ctx = sysdb_ctx_get_ldb(domain->sysdb);
@@ -840,6 +1106,13 @@ ad_gpo_process_som_done(struct tevent_req *subreq) /*
- This function retrieves a list of candidate_gpos and potentially reduces it
- to a list of dacl_filtered_gpos, based on each GPO's DACL.
- This function then takes the list of dacl_filtered_gpos and potentially
- reduces it to a list of cse_filtered_gpos, based on whether each GPO's list
- of cse_guids includes the "SecuritySettings" CSE GUID (used for HBAC).
- This function then sends each cse_filtered_gpo to the CSE processing engine
*/
- for policy application, which currently consists of HBAC functionality.
static void ad_gpo_process_gpo_done(struct tevent_req *subreq) @@ -878,7 +1151,7 @@ ad_gpo_process_gpo_done(struct tevent_req *subreq) &state->num_dacl_filtered_gpos); if (ret != EOK) { DEBUG(SSSDBG_OP_FAILURE,
"Unable to filter GPO list: [%d](%s)\n",
"Unable to filter GPO list by DACKL: [%d](%s)\n",
I guess this ^ is a typo to make the German reviewer smile (https://de.wikipedia.org/wiki/Dackel :-).
ret, sss_strerror(ret)); goto done; }
@@ -896,10 +1169,133 @@ ad_gpo_process_gpo_done(struct tevent_req *subreq) state->dacl_filtered_gpos[i]->gpo_guid); }
- /* TBD: initiate SMB retrieval */
- DEBUG(SSSDBG_TRACE_FUNC, "time for SMB retrieval\n");
- ret = ad_gpo_filter_gpos_by_cse_guid(state,
GP_EXT_GUID_SECURITY,
state->dacl_filtered_gpos,
state->num_dacl_filtered_gpos,
&state->cse_filtered_gpos,
&state->num_cse_filtered_gpos);
- ret = EOK;
- if (ret != EOK) {
DEBUG(SSSDBG_OP_FAILURE,
"Unable to filter GPO list by CSE_GUID: [%d](%s)\n",
ret, strerror(ret));
goto done;
- }
- if (state->cse_filtered_gpos[0] == NULL) {
/* no gpos contain "SecuritySettings" cse_guid, nothing to enforce */
DEBUG(SSSDBG_TRACE_FUNC,
"no applicable gpos found after cse_guid filtering\n");
ret = EOK;
goto done;
- }
- for (i = 0; i < state->num_cse_filtered_gpos; i++) {
DEBUG(SSSDBG_TRACE_FUNC, "cse_filtered_gpos[%d]->gpo_guid is %s\n", i,
state->cse_filtered_gpos[i]->gpo_guid);
- }
- DEBUG(SSSDBG_TRACE_FUNC, "num_cse_filtered_gpos: %d\n",
state->num_cse_filtered_gpos);
- ret = ad_gpo_cse_step(req);
- done:
- if (ret == EOK) {
tevent_req_done(req);
- } else if (ret != EAGAIN) {
tevent_req_error(req, ret);
- }
+}
+static errno_t +ad_gpo_cse_step(struct tevent_req *req) +{
- struct tevent_req *subreq;
- struct ad_gpo_access_state *state;
- char *smb_uri;
- int i = 0;
- state = tevent_req_data(req, struct ad_gpo_access_state);
- state->cse_gpo_index++;
- struct gp_gpo *cse_filtered_gpo =
state->cse_filtered_gpos[state->cse_gpo_index];
- /* cse_filtered_gpo is NULL only after all GPOs have been processed */
- if (cse_filtered_gpo == NULL) return EOK;
- DEBUG(SSSDBG_TRACE_FUNC, "cse filtered_gpos[%d]->gpo_guid is %s\n",
state->cse_gpo_index, cse_filtered_gpo->gpo_guid);
- DEBUG(SSSDBG_TRACE_FUNC, "cse filtered_gpos[%d]->file_sys_path is %s\n",
state->cse_gpo_index, cse_filtered_gpo->gpo_file_sys_path);
- for (i = 0; i < cse_filtered_gpo->num_gpo_cse_guids; i++) {
DEBUG(SSSDBG_TRACE_ALL,
"cse_filtered_gpos[%d]->gpo_cse_guids[%d]->gpo_guid is %s\n",
state->cse_gpo_index, i, cse_filtered_gpo->gpo_cse_guids[i]);
- }
- smb_uri = talloc_asprintf(state, "%s%s",
cse_filtered_gpo->gpo_file_sys_path,
GP_EXT_GUID_SECURITY_SUFFIX);
- subreq = ad_gpo_process_cse_send(state, state->ev, smb_uri, cse_filtered_gpo);
- tevent_req_set_callback(subreq, ad_gpo_cse_done, req);
- return EAGAIN;
+}
+/*
- This cse-specific function (GP_EXT_GUID_SECURITY) retrieves a list of
- allowed_sids and denied_sids, and uses them to determine whether logon
- access is granted or denied for the state's {user, domain} tuple.
- If it is determined that the current cse_filtered_gpo grants access, then
- we process the next cse_filtered_gpo in the list. At any time, if access is
- denied, we return immediately with an error.
- */
+static void +ad_gpo_cse_done(struct tevent_req *subreq) +{
- struct tevent_req *req;
- struct ad_gpo_access_state *state;
- int ret;
- char **allowed_sids;
- int allowed_size;
- char **denied_sids;
- int denied_size;
- req = tevent_req_callback_data(subreq, struct tevent_req);
- state = tevent_req_data(req, struct ad_gpo_access_state);
- ret = ad_gpo_process_cse_recv(subreq, state, &allowed_sids, &allowed_size,
&denied_sids, &denied_size);
- talloc_zfree(subreq);
- if (ret != EOK) {
/* TBD: handle ret error */
goto done;
- }
- /* TBD: allowed/denied_sids/size, should be retrieved from cache */
- ret = ad_gpo_access_check
(state, state->user, state->domain,
allowed_sids, allowed_size, denied_sids, denied_size);
- if (ret == EOK) {
DEBUG(SSSDBG_CRIT_FAILURE, "ret is EOK\n");
I think this should better be a SSSDBG_TRACE_ALL if needed at all.
- } else {
DEBUG(SSSDBG_CRIT_FAILURE, "ret is not EOK\n");
Please make the message a bit more verbose.
DEBUG(SSSDBG_OP_FAILURE,
"GPO check failed: [%d](%s)\n",
ret, strerror(ret));
goto done;
}
ret = ad_gpo_cse_step(req);
done:
@@ -2307,3 +2703,278 @@ ad_gpo_process_gpo_recv(struct tevent_req *req, *num_candidate_gpos = state->num_candidate_gpos; return EOK; }
+/* == ad_gpo_process_cse_send/recv helpers ================================= */
+/*
- This function uses the input ini_config object to parse the logon right value
- associated with the input name. This value is a list of sids, and is used
- to populate the output parameters. The input name can be either
- ALLOW_LOGON_LOCALLY or DENY_LOGON_LOCALLY.
- */
+static errno_t +parse_logon_right_with_libini(struct ini_cfgobj *ini_config,
const char *name,
int *_size,
char ***_sids)
+{
- int ret = 0;
- struct value_obj *vobj = NULL;
- char **sids = NULL;
- int num_sids = 0;
- int i;
- ret = ini_get_config_valueobj(RIGHTS_SECTION, name, ini_config,
INI_GET_FIRST_VALUE, &vobj);
- if (vobj == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "section/name not found: [%s][%s]\n",
RIGHTS_SECTION, name);
return EOK;
- }
- sids = ini_get_string_config_array(vobj, NULL, &num_sids, &ret);
- if (ret) {
Please do explicit checks, in this case (ret != 0) because according to the docs ini_get_string_config_array() returns 0 on success.
DEBUG(SSSDBG_CRIT_FAILURE,
"ini_get_string_config_array failed [%d][%s]\n", ret, strerror(ret));
return ret;
- }
- for (i = 0; i <num_sids; i++) {
- if (sids[i][0] == '*') {
sids[i]++;
- }
- }
What is this loop good for?
- *_size = num_sids;
- *_sids = sids;
- return EOK;
+}
+/*
- This function parses the cse-specific (GP_EXT_GUID_SECURITY) input data_buf,
- and uses the results to populate the output parameters with the list of
- allowed_sids and denied_sids
- */
+static errno_t +ad_gpo_parse_security_cse_buffer(uint8_t *data_buf,
int data_len,
char ***allowed_sids,
int *allowed_size,
char ***denied_sids,
int *denied_size)
+{
- struct ini_cfgfile *file_ctx = NULL;
- struct ini_cfgobj *ini_config = NULL;
- int ret;
- char **allow_sids = NULL; char **deny_sids = NULL;
- int allow_size = 0; int deny_size = 0;
- ret = ini_config_create(&ini_config);
- if (ret) goto done;
see above (ret != 0)
- ret = ini_config_file_from_mem(data_buf, data_len, &file_ctx);
- if (ret) goto done;
see above
- ret = ini_config_parse(file_ctx, INI_STOP_ON_NONE, 0, 0, ini_config);
- if (ret) goto done;
see above
- ret = parse_logon_right_with_libini(ini_config,
ALLOW_LOGON_LOCALLY,
&allow_size,
&allow_sids);
- if (ret) {goto done;}
see above, here (ret != EOK) would be good as well because parse_logon_right_with_libini() returns EOK on success.
- ret = parse_logon_right_with_libini(ini_config,
DENY_LOGON_LOCALLY,
&deny_size,
&deny_sids);
- if (ret) {goto done;}
see above
- *allowed_sids = allow_sids;
- *allowed_size = allow_size;
- *denied_sids = deny_sids;
- *denied_size = deny_size;
- done:
- if (ret) {
see above
DEBUG(SSSDBG_CRIT_FAILURE, "Error encountered: %d.\n", ret);
- }
- ini_config_file_close(file_ctx);
- return ret;
+}
+static void +sssd_krb_get_auth_data_fn(const char * pServer,
const char * pShare,
char * pWorkgroup,
int maxLenWorkgroup,
char * pUsername,
int maxLenUsername,
char * pPassword,
int maxLenPassword)
+{
- /* since we are using kerberos for authentication, we simply return */
- return;
+}
+/*
- This cse-specific function (GP_EXT_GUID_SECURITY) opens an SMB connection,
- retrieves the data referenced by the input smb_uri, and then closes the SMB
- connection. The data is then parsed and the results are used to populate the
- output parameters with the list of allowed_sids and denied_sids
- */
+static errno_t +process_security_settings_cse(TALLOC_CTX *mem_ctx,
const char *smb_uri,
char ***_allowed_sids,
int *_allowed_size,
char ***_denied_sids,
int *_denied_size)
+{
- SMBCCTX *context;
- int ret = 0;
- uint8_t *buf = NULL;
- int bytesread = 0;
- char **allowed_sids;
- char **denied_sids;
- int allowed_size = 0;
- int denied_size = 0;
- DEBUG(SSSDBG_TRACE_ALL, "%s\n", smb_uri);
- context = smbc_new_context();
- if (!context) {
DEBUG(SSSDBG_CRIT_FAILURE, "Could not allocate new smbc context\n");
- }
- smbc_setFunctionAuthData(context, sssd_krb_get_auth_data_fn);
- smbc_setOptionUseKerberos(context, 1);
- /* Initialize the context using the previously specified options */
- if (!smbc_init_context(context)) {
smbc_free_context(context, 0);
DEBUG(SSSDBG_CRIT_FAILURE, "Could not initialize smbc context\n");
- }
- /* Tell the compatibility layer to use this context */
- smbc_set_context(context);
- int remotehandle = smbc_open(smb_uri, O_RDONLY, 0755);
- if (remotehandle < 0) DEBUG(SSSDBG_CRIT_FAILURE, "smbc_open failed\n");
- buf = talloc_array(mem_ctx, uint8_t, SMB_BUFFER_SIZE);
- bytesread = smbc_read(remotehandle, buf, SMB_BUFFER_SIZE);
- if(bytesread < 0) DEBUG(SSSDBG_CRIT_FAILURE, "smbc_read failed\n");
- DEBUG(SSSDBG_CRIT_FAILURE, "bytesread: %d\n", bytesread);
- smbc_close(remotehandle);
- ret = ad_gpo_parse_security_cse_buffer(buf,
bytesread,
&allowed_sids,
&allowed_size,
&denied_sids,
&denied_size);
- /* TBD: allowed/denied_sids/size should be stored in cache */
- *_allowed_sids = allowed_sids;
- *_allowed_size = allowed_size;
- *_denied_sids = denied_sids;
- *_denied_size = denied_size;
- return ret;
+}
+/* == ad_gpo_process_cse_send/recv implementation ========================== */
+struct ad_gpo_process_cse_state {
- struct tevent_context *ev;
- char **allowed_sids;
- int allowed_size;
- char **denied_sids;
- int denied_size;
- struct gp_gpo *cse_filtered_gpo;
+};
+/*
- This cse-specific function (GP_EXT_GUID_SECURITY) retrieves the data
- referenced by the input smb_uri, and uses the parsed results to populate the
- state's list of allowed_sids and denied_sids.
- */
+struct tevent_req * +ad_gpo_process_cse_send(TALLOC_CTX *mem_ctx,
struct tevent_context *ev,
char *smb_uri,
struct gp_gpo *cse_filtered_gpo)
+{
- struct tevent_req *req;
- struct ad_gpo_process_cse_state *state;
- errno_t ret;
- char **allowed_sids;
- int allowed_size;
- char **denied_sids;
- int denied_size;
- req = tevent_req_create(mem_ctx, &state, struct ad_gpo_process_cse_state);
- if (req == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "tevent_req_create() failed\n");
return NULL;
- }
- state->ev = ev;
- state->cse_filtered_gpo = cse_filtered_gpo;
- ret = process_security_settings_cse
(state, smb_uri, &allowed_sids, &allowed_size,
&denied_sids, &denied_size);
- if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE, "process_security_settings_cse FAILED\n");
goto done;
- }
- state->allowed_sids = allowed_sids;
- state->allowed_size = allowed_size;
- state->denied_sids = denied_sids;
- state->denied_size = denied_size;
- ret = EOK;
- done:
- if (ret == EOK) {
tevent_req_done(req);
tevent_req_post(req, ev);
- } else {
tevent_req_error(req, ret);
tevent_req_post(req, ev);
- }
You can extract tevent_req_post() and run it unconditionally after the if block.
- return req;
+}
+int ad_gpo_process_cse_recv(struct tevent_req *req,
TALLOC_CTX *mem_ctx,
char ***allowed_sids,
int *allowed_size,
char ***denied_sids,
int *denied_size)
+{
- struct ad_gpo_process_cse_state *state =
tevent_req_data(req, struct ad_gpo_process_cse_state);
- TEVENT_REQ_RETURN_ON_ERROR(req);
- *allowed_sids = state->allowed_sids;
- *allowed_size = state->allowed_size;
I guess talloc_steal is missing here.
- *denied_sids = state->denied_sids;
- *denied_size = state->denied_size;
- return EOK;
+}
1.8.5.3
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
----- Original Message -----
On Fri, May 30, 2014 at 12:35:32PM -0400, Yassir Elley wrote:
----- Original Message -----
On (29/05/14 09:06), Yassir Elley wrote:
----- Original Message -----
On Thu, May 29, 2014 at 04:09:21AM -0400, Yassir Elley wrote:
Also note that some (but not all) of the changes that were pushed to master as part of my patch titled "add libsmbclient to makefiles" seem to have been deleted by subsequent pushes to master. As such, this patch reintroduces those changes (which only affect configure.ac and libsmbclient.m4).
That would be worrisome, what changes in particular? I'm not aware of pushing anything else than acked patches from the list..
Are you talking about the autoconf refactor Lukas did? IIRC that one only merged some code duplicates into a single file.. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Hi Jakub,
You are right. I overlooked the fact that the autoconf refactor had moved things around. My mistake.
Revised patch attached.
Regards, Yassir.
From abc4ac3e664ccbeeb58d131dc6c37cc314f5ba3d Mon Sep 17 00:00:00 2001 From: Yassir Elley yelley@redhat.com Date: Wed, 21 May 2014 15:11:36 -0400 Subject: [PATCH] AD-GPO: Add initial gpo-smb implementation
src/providers/ad/ad_gpo.c | 680 +++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 676 insertions(+), 4 deletions(-)
diff --git a/src/providers/ad/ad_gpo.c b/src/providers/ad/ad_gpo.c index 168ed2d7e8657799a7c773ec8b24897cdd303ec4..519c0dbec4d85e7f6e41db672ae0f2c2f720d81b 100644 --- a/src/providers/ad/ad_gpo.c +++ b/src/providers/ad/ad_gpo.c @@ -31,6 +31,9 @@ */
#include <security/pam_modules.h> +#include <libsmbclient.h> +#include <ini_configobj.h> +#include <ini_config.h>
^^^^^^^^^^^^
This is the old ini_config interface. You can remove this line.
#include "util/util.h" #include "util/strtonum.h" #include "providers/data_provider.h"
// snip
- int ret;
- char **allow_sids = NULL; char **deny_sids = NULL;
- int allow_size = 0; int deny_size = 0;
- ret = ini_config_create(&ini_config);
- if (ret) goto done;
- ret = ini_config_file_from_mem(data_buf, data_len, &file_ctx);
^^^^^^^^^^^^^^^^^^^^^^^^
This function is available in libini_config-1.1 Please, update buildrequires in spec file.
diff --git a/contrib/sssd.spec.in b/contrib/sssd.spec.in index b5e7903..10704ae 100644 --- a/contrib/sssd.spec.in +++ b/contrib/sssd.spec.in @@ -97,7 +97,7 @@ BuildRequires: libtdb-devel BuildRequires: libldb-devel BuildRequires: libdhash-devel >= 0.4.2 BuildRequires: libcollection-devel -BuildRequires: libini_config-devel +BuildRequires: libini_config-devel >= 1.1 BuildRequires: dbus-devel BuildRequires: dbus-libs %if 0%{?rhel5_minor} >= 7
LS
Revised patch attached.
Also, I successfully tested the code after installing the ding-libs packages from lslebodn's repo. Previously, I was using rpms that I had manually compiled (for the ini_config_file_from_mem functionality).
Thanks, Yassir.
I didn't had a chance to do functional testings yet, I'll send my results tomorrow. For a start please find my comments on the code inline.
Please let me know if you have additional comments after functional testing.
bye, Sumit
From 24625d6cd02faeb39e8522769d470b3a8beed513 Mon Sep 17 00:00:00 2001 From: Yassir Elley yelley@redhat.com Date: Wed, 21 May 2014 15:11:36 -0400 Subject: [PATCH] AD-GPO: Add initial gpo-smb implementation
contrib/sssd.spec.in | 2 +- src/providers/ad/ad_gpo.c | 679 +++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 676 insertions(+), 5 deletions(-)
diff --git a/contrib/sssd.spec.in b/contrib/sssd.spec.in index b5e7903713ccc6944fbed4f84c2f0413a257b760..10704ae48cf5636de2a5cbbb42fc00b99052144f 100644 --- a/contrib/sssd.spec.in +++ b/contrib/sssd.spec.in @@ -97,7 +97,7 @@ BuildRequires: libtdb-devel BuildRequires: libldb-devel BuildRequires: libdhash-devel >= 0.4.2 BuildRequires: libcollection-devel -BuildRequires: libini_config-devel +BuildRequires: libini_config-devel >= 1.1
Please add this new minimal version to the configure check as well.
I'm not exactly sure what you mean here? Can you please provide a diff?
BuildRequires: dbus-devel BuildRequires: dbus-libs %if 0%{?rhel5_minor} >= 7 diff --git a/src/providers/ad/ad_gpo.c b/src/providers/ad/ad_gpo.c index bb6896e70ae96036b1a9e58a6c8e25d3d8f1543f..9a0f09f9d2735158d970bb19d631645eb8ae9cfa 100644 --- a/src/providers/ad/ad_gpo.c +++ b/src/providers/ad/ad_gpo.c
...
@@ -543,6 +571,235 @@ ad_gpo_filter_gpos_by_dacl(TALLOC_CTX *mem_ctx, return ret; }
+/*
- This function determines whether the input cse_guid matches any of the
input
- gpo_cse_guids. The boolean result is assigned to the _included output
param.
- */
+static errno_t +ad_gpo_includes_cse_guid(const char *cse_guid,
const char **gpo_cse_guids,
int num_gpo_cse_guids,
bool *_included)
In general I prefer that a function returns an error code and results are returned by reference. But in this case no error is returned only EOK and I think it would be easier to just return the bool value directly. If you prefer to keep the error code, e.g. because you plan to add some NULL checks for cse_guid and gpo_cse_guids and return EINVAL in this case, please check the return value when calling ad_gpo_includes_cse_guid() below.
I will change it to return a bool value, since the NULL checks are not needed.
+{
- int i = 0;
- const char *gpo_cse_guid = NULL;
- for (i = 0; i < num_gpo_cse_guids; i++) {
gpo_cse_guid = gpo_cse_guids[i];
if (strcmp(gpo_cse_guid, cse_guid) == 0) {
*_included = true;
return EOK;
}
- }
- *_included = false;
- return EOK;
+}
+/*
- This function takes an input dacl_filtered_gpos list, filters out any
gpo
- that does not contain the input cse_guid, and assigns the result to the
- _cse_filtered_gpos output parameter.
- */
+static errno_t +ad_gpo_filter_gpos_by_cse_guid(TALLOC_CTX *mem_ctx,
const char *cse_guid,
struct gp_gpo **dacl_filtered_gpos,
int num_dacl_filtered_gpos,
struct gp_gpo ***_cse_filtered_gpos,
int *_num_cse_filtered_gpos)
+{
- TALLOC_CTX *tmp_ctx = NULL;
- int i = 0;
- int ret = 0;
- struct gp_gpo *dacl_filtered_gpo = NULL;
- int gpo_dn_idx = 0;
- struct gp_gpo **cse_filtered_gpos = NULL;
- bool included;
- tmp_ctx = talloc_new(NULL);
- if (tmp_ctx == NULL) {
ret = ENOMEM;
goto done;
- }
- cse_filtered_gpos = talloc_array(tmp_ctx,
struct gp_gpo *,
num_dacl_filtered_gpos + 1);
- if (cse_filtered_gpos == NULL) {
ret = ENOMEM;
goto done;
- }
- for (i = 0; i < num_dacl_filtered_gpos; i++) {
dacl_filtered_gpo = dacl_filtered_gpos[i];
DEBUG(SSSDBG_TRACE_ALL, "examining cse candidate_gpo_guid: %s\n",
dacl_filtered_gpo->gpo_guid);
ret = ad_gpo_includes_cse_guid(cse_guid,
dacl_filtered_gpo->gpo_cse_guids,
dacl_filtered_gpo->num_gpo_cse_guids,
&included);
see above
if (included) {
DEBUG(SSSDBG_TRACE_ALL,
"GPO applicable to target per cse_guid filtering\n");
cse_filtered_gpos[gpo_dn_idx] =
talloc_steal(cse_filtered_gpos,
dacl_filtered_gpo);
Do you need the item still in dacl_filtered_gpos? If not I would prefer to set dacl_filtered_gpos[i] to NULL or call talloc_move on it. If it is still needed in dacl_filtered_gpos I wonder if it would be safer to copy the item to avoid that someone iterates over the dacl_filtered_gpos elements and frees them individually?
The item in dacl_filtered_gpos is no longer necessary. I will set dacl_filtered_gpos[i] to NULL.
gpo_dn_idx++;
} else {
DEBUG(SSSDBG_TRACE_ALL,
"GPO not applicable to target per cse_guid
filtering\n");
continue;
}
- }
- cse_filtered_gpos[gpo_dn_idx] = NULL;
- *_cse_filtered_gpos = talloc_steal(mem_ctx, cse_filtered_gpos);
- *_num_cse_filtered_gpos = gpo_dn_idx;
- ret = EOK;
- done:
- talloc_free(tmp_ctx);
- return ret;
+}
+/*
- This cse-specific function (GP_EXT_GUID_SECURITY) populates the output
- parameter (found) based on whether the input user_sid or any of the
input
- group_sids appear in the input list of privilege_sids.
- */
+static void +check_rights(char **privilege_sids,
int privilege_size,
const char *user_sid,
const char **group_sids,
int group_size,
int *found)
I would suggest to return 'found' directly here as well and maybe convert it to a bool or is there a reason for having an int here?
I will return a bool directly here as well.
+{
- int match = 0;
- int i, j;
- for (i = 0; i < privilege_size; i++) {
- if (strcmp(user_sid, privilege_sids[i]) == 0) {
match = 1;
break;
- }
- for (j = 0; j < group_size; j++) {
if (strcmp(group_sids[j], privilege_sids[i]) == 0) {
- match = 1;
- break;
^ Tab indent
}
- }
- }
- *found = match;
+}
+/*
- This cse-specific function (GP_EXT_GUID_SECURITY) performs HBAC policy
- application and determines whether logon access is granted or denied
for
- the {user,domain} tuple specified in the inputs. This function returns
EOK
- to indicate that access is granted. Any other return value indicates
that
- access is denied.
- The access control algorithm first determines whether the
"principal_sids"
- (i.e. user_sid or group_sids) appear in allowed_sids and denied_sids.
- For access to be granted, both the "allowed_sids_condition" *and* the
- "denied_sids_condition" must be met (in all other cases, access is
denied).
- The "allowed_sids_condition" is satisfied if any of the
principal_sids
- appears in allowed_sids OR if the allowed_sids list is empty
- The "denied_sids_condition" is satisfied if none of the
principal_sids
- appear in denied_sids
- Note that a deployment that is unaware of GPO-based access-control
policy
- settings is unaffected by them (b/c the absence of allowed_sids grants
access).
- Note that if a principal_sid appears in both allowed_sids and
denied_sids,
- the "allowed_sids_condition" is met, but the "denied_sids_condition" is
not.
- In other words, Deny takes precedence over Allow.
I have two comments here. First, is there a way to avoid having deny rules? The reason is that we had some troubles in the past here, because when checking deny rules all group memberships of a user must be available to properly deny access. If some are missing access might be granted although it should be rejected. I guess it is not possible to get rid of them because they will be set on the AD side and we have to respect them.
I agree that relying on deny rules for security is problematic, b/c it can be difficult to exhaustively prove a negative. Don't we have the same challenge with the simple_access_provider's simple_deny_groups?
In any case, I also agree with your conclusion that we have to respect the GPO policy settings defined on the AD side.
Second, you are only checking SeInteractiveLogonRight and SeDenyInteractiveLogonRight. What about Se[Deny][Network|Batch|Service|RemoteInteractive]LogonRight? Are you planning to integrate them as well and relate them to different PAM services?
The plan for the initial deployment is to only support SeInteractiveLogonRight and SeDenyInteractiveLogonRight.
- */
+static errno_t +ad_gpo_access_check(TALLOC_CTX *mem_ctx,
const char *user,
struct sss_domain_info *domain,
char **allowed_sids,
int allowed_size,
char **denied_sids,
int denied_size)
+{
- const char *user_sid;
- const char **group_sids;
- int group_size = 0;
- int access_granted = 0;
- int access_denied = 0;
Is there a reason to use int instead of bool?
No. I have made the change.
- int ret;
- int j;
- DEBUG(SSSDBG_TRACE_FUNC, "POLICY FILE:\n");
- DEBUG(SSSDBG_TRACE_FUNC, "allowed_size = %d\n", allowed_size);
- for (j= 0; j < allowed_size; j++) {
DEBUG(SSSDBG_TRACE_FUNC, "allowed_sids[%d] = %s\n", j,
allowed_sids[j]);
- }
- DEBUG(SSSDBG_TRACE_FUNC, "denied_size = %d\n", denied_size);
- for (j= 0; j < denied_size; j++) {
DEBUG(SSSDBG_TRACE_FUNC, " denied_sids[%d] = %s\n", j,
denied_sids[j]);
- }
- ret = ad_gpo_get_sids(mem_ctx, user, domain, &user_sid,
&group_sids, &group_size);
- if (ret != EOK) {
ret = ERR_NO_SIDS;
DEBUG(SSSDBG_OP_FAILURE,
"Unable to retrieve SIDs: [%d](%s)\n", ret,
sss_strerror(ret));
goto done;
- }
- DEBUG(SSSDBG_TRACE_FUNC, "CURRENT USER:\n");
- DEBUG(SSSDBG_TRACE_FUNC, " user_sid = %s\n", user_sid);
- for (j= 0; j < group_size; j++) {
DEBUG(SSSDBG_TRACE_FUNC, " group_sids[%d] = %s\n", j,
group_sids[j]);
- }
- /* If AllowLogonLocally is not defined, all users are allowed */
- if (allowed_size == 0) {
access_granted = 1;
- } else {
check_rights(allowed_sids, allowed_size, user_sid,
group_sids, group_size, &access_granted);
- }
- DEBUG(SSSDBG_TRACE_FUNC, " access_granted = %d\n", access_granted);
- check_rights(denied_sids, denied_size, user_sid,
group_sids, group_size, &access_denied);
- DEBUG(SSSDBG_TRACE_FUNC, " access_denied = %d\n", access_denied);
- if (access_granted && !access_denied) {
return EOK;
- } else {
return EACCES;
- }
- done:
- if (ret) {
DEBUG(SSSDBG_CRIT_FAILURE, "Error encountered: %d.\n", ret);
- }
- return ret;
+}
/* == ad_gpo_access_send/recv implementation ================================*/
struct ad_gpo_access_state { @@ -558,6 +815,9 @@ struct ad_gpo_access_state { const char *target_dn; struct gp_gpo **dacl_filtered_gpos; int num_dacl_filtered_gpos;
- struct gp_gpo **cse_filtered_gpos;
- int num_cse_filtered_gpos;
- int cse_gpo_index;
};
static void ad_gpo_connect_done(struct tevent_req *subreq); @@ -565,6 +825,9 @@ static void ad_gpo_target_dn_retrieval_done(struct tevent_req *subreq); static void ad_gpo_process_som_done(struct tevent_req *subreq); static void ad_gpo_process_gpo_done(struct tevent_req *subreq);
+static errno_t ad_gpo_cse_step(struct tevent_req *req); +static void ad_gpo_cse_done(struct tevent_req *subreq);
struct tevent_req * ad_gpo_access_send(TALLOC_CTX *mem_ctx, struct tevent_context *ev, @@ -586,6 +849,9 @@ ad_gpo_access_send(TALLOC_CTX *mem_ctx, state->domain = domain; state->dacl_filtered_gpos = NULL; state->num_dacl_filtered_gpos = 0;
- state->cse_filtered_gpos = NULL;
- state->num_cse_filtered_gpos = 0;
- state->cse_gpo_index = -1; state->ev = ev; state->user = user; state->ldb_ctx = sysdb_ctx_get_ldb(domain->sysdb);
@@ -840,6 +1106,13 @@ ad_gpo_process_som_done(struct tevent_req *subreq) /*
- This function retrieves a list of candidate_gpos and potentially
reduces it
- to a list of dacl_filtered_gpos, based on each GPO's DACL.
- This function then takes the list of dacl_filtered_gpos and potentially
- reduces it to a list of cse_filtered_gpos, based on whether each GPO's
list
- of cse_guids includes the "SecuritySettings" CSE GUID (used for HBAC).
- This function then sends each cse_filtered_gpo to the CSE processing
engine
*/
- for policy application, which currently consists of HBAC functionality.
static void ad_gpo_process_gpo_done(struct tevent_req *subreq) @@ -878,7 +1151,7 @@ ad_gpo_process_gpo_done(struct tevent_req *subreq) &state->num_dacl_filtered_gpos); if (ret != EOK) { DEBUG(SSSDBG_OP_FAILURE,
"Unable to filter GPO list: [%d](%s)\n",
"Unable to filter GPO list by DACKL: [%d](%s)\n",
I guess this ^ is a typo to make the German reviewer smile (https://de.wikipedia.org/wiki/Dackel :-).
Just for you, Sumit :)
ret, sss_strerror(ret)); goto done; }
@@ -896,10 +1169,133 @@ ad_gpo_process_gpo_done(struct tevent_req *subreq) state->dacl_filtered_gpos[i]->gpo_guid); }
- /* TBD: initiate SMB retrieval */
- DEBUG(SSSDBG_TRACE_FUNC, "time for SMB retrieval\n");
- ret = ad_gpo_filter_gpos_by_cse_guid(state,
GP_EXT_GUID_SECURITY,
state->dacl_filtered_gpos,
state->num_dacl_filtered_gpos,
&state->cse_filtered_gpos,
&state->num_cse_filtered_gpos);
- ret = EOK;
- if (ret != EOK) {
DEBUG(SSSDBG_OP_FAILURE,
"Unable to filter GPO list by CSE_GUID: [%d](%s)\n",
ret, strerror(ret));
goto done;
- }
- if (state->cse_filtered_gpos[0] == NULL) {
/* no gpos contain "SecuritySettings" cse_guid, nothing to enforce
*/
DEBUG(SSSDBG_TRACE_FUNC,
"no applicable gpos found after cse_guid filtering\n");
ret = EOK;
goto done;
- }
- for (i = 0; i < state->num_cse_filtered_gpos; i++) {
DEBUG(SSSDBG_TRACE_FUNC, "cse_filtered_gpos[%d]->gpo_guid is
%s\n", i,
state->cse_filtered_gpos[i]->gpo_guid);
- }
- DEBUG(SSSDBG_TRACE_FUNC, "num_cse_filtered_gpos: %d\n",
state->num_cse_filtered_gpos);
- ret = ad_gpo_cse_step(req);
- done:
- if (ret == EOK) {
tevent_req_done(req);
- } else if (ret != EAGAIN) {
tevent_req_error(req, ret);
- }
+}
+static errno_t +ad_gpo_cse_step(struct tevent_req *req) +{
- struct tevent_req *subreq;
- struct ad_gpo_access_state *state;
- char *smb_uri;
- int i = 0;
- state = tevent_req_data(req, struct ad_gpo_access_state);
- state->cse_gpo_index++;
- struct gp_gpo *cse_filtered_gpo =
state->cse_filtered_gpos[state->cse_gpo_index];
- /* cse_filtered_gpo is NULL only after all GPOs have been processed */
- if (cse_filtered_gpo == NULL) return EOK;
- DEBUG(SSSDBG_TRACE_FUNC, "cse filtered_gpos[%d]->gpo_guid is %s\n",
state->cse_gpo_index, cse_filtered_gpo->gpo_guid);
- DEBUG(SSSDBG_TRACE_FUNC, "cse filtered_gpos[%d]->file_sys_path is
%s\n",
state->cse_gpo_index, cse_filtered_gpo->gpo_file_sys_path);
- for (i = 0; i < cse_filtered_gpo->num_gpo_cse_guids; i++) {
DEBUG(SSSDBG_TRACE_ALL,
"cse_filtered_gpos[%d]->gpo_cse_guids[%d]->gpo_guid is
%s\n",
state->cse_gpo_index, i,
cse_filtered_gpo->gpo_cse_guids[i]);
- }
- smb_uri = talloc_asprintf(state, "%s%s",
cse_filtered_gpo->gpo_file_sys_path,
GP_EXT_GUID_SECURITY_SUFFIX);
- subreq = ad_gpo_process_cse_send(state, state->ev, smb_uri,
cse_filtered_gpo);
- tevent_req_set_callback(subreq, ad_gpo_cse_done, req);
- return EAGAIN;
+}
+/*
- This cse-specific function (GP_EXT_GUID_SECURITY) retrieves a list of
- allowed_sids and denied_sids, and uses them to determine whether logon
- access is granted or denied for the state's {user, domain} tuple.
- If it is determined that the current cse_filtered_gpo grants access,
then
- we process the next cse_filtered_gpo in the list. At any time, if
access is
- denied, we return immediately with an error.
- */
+static void +ad_gpo_cse_done(struct tevent_req *subreq) +{
- struct tevent_req *req;
- struct ad_gpo_access_state *state;
- int ret;
- char **allowed_sids;
- int allowed_size;
- char **denied_sids;
- int denied_size;
- req = tevent_req_callback_data(subreq, struct tevent_req);
- state = tevent_req_data(req, struct ad_gpo_access_state);
- ret = ad_gpo_process_cse_recv(subreq, state, &allowed_sids,
&allowed_size,
&denied_sids, &denied_size);
- talloc_zfree(subreq);
- if (ret != EOK) {
/* TBD: handle ret error */
goto done;
- }
- /* TBD: allowed/denied_sids/size, should be retrieved from cache */
- ret = ad_gpo_access_check
(state, state->user, state->domain,
allowed_sids, allowed_size, denied_sids, denied_size);
- if (ret == EOK) {
DEBUG(SSSDBG_CRIT_FAILURE, "ret is EOK\n");
I think this should better be a SSSDBG_TRACE_ALL if needed at all.
- } else {
DEBUG(SSSDBG_CRIT_FAILURE, "ret is not EOK\n");
Please make the message a bit more verbose.
These two debug messages were left over from my initial troubleshooting during development. I have removed them both.
DEBUG(SSSDBG_OP_FAILURE,
"GPO check failed: [%d](%s)\n",
ret, strerror(ret));
goto done;
}
ret = ad_gpo_cse_step(req);
done:
@@ -2307,3 +2703,278 @@ ad_gpo_process_gpo_recv(struct tevent_req *req, *num_candidate_gpos = state->num_candidate_gpos; return EOK; }
+/* == ad_gpo_process_cse_send/recv helpers ================================= */
+/*
- This function uses the input ini_config object to parse the logon right
value
- associated with the input name. This value is a list of sids, and is
used
- to populate the output parameters. The input name can be either
- ALLOW_LOGON_LOCALLY or DENY_LOGON_LOCALLY.
- */
+static errno_t +parse_logon_right_with_libini(struct ini_cfgobj *ini_config,
const char *name,
int *_size,
char ***_sids)
+{
- int ret = 0;
- struct value_obj *vobj = NULL;
- char **sids = NULL;
- int num_sids = 0;
- int i;
- ret = ini_get_config_valueobj(RIGHTS_SECTION, name, ini_config,
INI_GET_FIRST_VALUE, &vobj);
- if (vobj == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "section/name not found: [%s][%s]\n",
RIGHTS_SECTION, name);
return EOK;
- }
- sids = ini_get_string_config_array(vobj, NULL, &num_sids, &ret);
- if (ret) {
Please do explicit checks, in this case (ret != 0) because according to the docs ini_get_string_config_array() returns 0 on success.
Doesn't C define "true" as a non-zero value? Or are you suggesting this just for clarity?
DEBUG(SSSDBG_CRIT_FAILURE,
"ini_get_string_config_array failed [%d][%s]\n", ret,
strerror(ret));
return ret;
- }
- for (i = 0; i <num_sids; i++) {
- if (sids[i][0] == '*') {
sids[i]++;
- }
- }
What is this loop good for?
The GptTmpl.inf policy files I have seen all include an asterisk before the user_sid or group_sid; this loop removes the asterisk so that the policy file sids can be correctly compared against the cached sids (which don't have asterisks).
- *_size = num_sids;
- *_sids = sids;
- return EOK;
+}
+/*
- This function parses the cse-specific (GP_EXT_GUID_SECURITY) input
data_buf,
- and uses the results to populate the output parameters with the list of
- allowed_sids and denied_sids
- */
+static errno_t +ad_gpo_parse_security_cse_buffer(uint8_t *data_buf,
int data_len,
char ***allowed_sids,
int *allowed_size,
char ***denied_sids,
int *denied_size)
+{
- struct ini_cfgfile *file_ctx = NULL;
- struct ini_cfgobj *ini_config = NULL;
- int ret;
- char **allow_sids = NULL; char **deny_sids = NULL;
- int allow_size = 0; int deny_size = 0;
- ret = ini_config_create(&ini_config);
- if (ret) goto done;
see above (ret != 0)
- ret = ini_config_file_from_mem(data_buf, data_len, &file_ctx);
- if (ret) goto done;
see above
- ret = ini_config_parse(file_ctx, INI_STOP_ON_NONE, 0, 0, ini_config);
- if (ret) goto done;
see above
- ret = parse_logon_right_with_libini(ini_config,
ALLOW_LOGON_LOCALLY,
&allow_size,
&allow_sids);
- if (ret) {goto done;}
see above, here (ret != EOK) would be good as well because parse_logon_right_with_libini() returns EOK on success.
- ret = parse_logon_right_with_libini(ini_config,
DENY_LOGON_LOCALLY,
&deny_size,
&deny_sids);
- if (ret) {goto done;}
see above
- *allowed_sids = allow_sids;
- *allowed_size = allow_size;
- *denied_sids = deny_sids;
- *denied_size = deny_size;
- done:
- if (ret) {
see above
DEBUG(SSSDBG_CRIT_FAILURE, "Error encountered: %d.\n", ret);
- }
- ini_config_file_close(file_ctx);
- return ret;
+}
+static void +sssd_krb_get_auth_data_fn(const char * pServer,
const char * pShare,
char * pWorkgroup,
int maxLenWorkgroup,
char * pUsername,
int maxLenUsername,
char * pPassword,
int maxLenPassword)
+{
- /* since we are using kerberos for authentication, we simply return */
- return;
+}
+/*
- This cse-specific function (GP_EXT_GUID_SECURITY) opens an SMB
connection,
- retrieves the data referenced by the input smb_uri, and then closes the
SMB
- connection. The data is then parsed and the results are used to
populate the
- output parameters with the list of allowed_sids and denied_sids
- */
+static errno_t +process_security_settings_cse(TALLOC_CTX *mem_ctx,
const char *smb_uri,
char ***_allowed_sids,
int *_allowed_size,
char ***_denied_sids,
int *_denied_size)
+{
- SMBCCTX *context;
- int ret = 0;
- uint8_t *buf = NULL;
- int bytesread = 0;
- char **allowed_sids;
- char **denied_sids;
- int allowed_size = 0;
- int denied_size = 0;
- DEBUG(SSSDBG_TRACE_ALL, "%s\n", smb_uri);
- context = smbc_new_context();
- if (!context) {
DEBUG(SSSDBG_CRIT_FAILURE, "Could not allocate new smbc
context\n");
- }
- smbc_setFunctionAuthData(context, sssd_krb_get_auth_data_fn);
- smbc_setOptionUseKerberos(context, 1);
- /* Initialize the context using the previously specified options */
- if (!smbc_init_context(context)) {
smbc_free_context(context, 0);
DEBUG(SSSDBG_CRIT_FAILURE, "Could not initialize smbc context\n");
- }
- /* Tell the compatibility layer to use this context */
- smbc_set_context(context);
- int remotehandle = smbc_open(smb_uri, O_RDONLY, 0755);
- if (remotehandle < 0) DEBUG(SSSDBG_CRIT_FAILURE, "smbc_open
failed\n");
- buf = talloc_array(mem_ctx, uint8_t, SMB_BUFFER_SIZE);
- bytesread = smbc_read(remotehandle, buf, SMB_BUFFER_SIZE);
- if(bytesread < 0) DEBUG(SSSDBG_CRIT_FAILURE, "smbc_read failed\n");
- DEBUG(SSSDBG_CRIT_FAILURE, "bytesread: %d\n", bytesread);
- smbc_close(remotehandle);
- ret = ad_gpo_parse_security_cse_buffer(buf,
bytesread,
&allowed_sids,
&allowed_size,
&denied_sids,
&denied_size);
- /* TBD: allowed/denied_sids/size should be stored in cache */
- *_allowed_sids = allowed_sids;
- *_allowed_size = allowed_size;
- *_denied_sids = denied_sids;
- *_denied_size = denied_size;
- return ret;
+}
+/* == ad_gpo_process_cse_send/recv implementation ========================== */
+struct ad_gpo_process_cse_state {
- struct tevent_context *ev;
- char **allowed_sids;
- int allowed_size;
- char **denied_sids;
- int denied_size;
- struct gp_gpo *cse_filtered_gpo;
+};
+/*
- This cse-specific function (GP_EXT_GUID_SECURITY) retrieves the data
- referenced by the input smb_uri, and uses the parsed results to
populate the
- state's list of allowed_sids and denied_sids.
- */
+struct tevent_req * +ad_gpo_process_cse_send(TALLOC_CTX *mem_ctx,
struct tevent_context *ev,
char *smb_uri,
struct gp_gpo *cse_filtered_gpo)
+{
- struct tevent_req *req;
- struct ad_gpo_process_cse_state *state;
- errno_t ret;
- char **allowed_sids;
- int allowed_size;
- char **denied_sids;
- int denied_size;
- req = tevent_req_create(mem_ctx, &state, struct
ad_gpo_process_cse_state);
- if (req == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "tevent_req_create() failed\n");
return NULL;
- }
- state->ev = ev;
- state->cse_filtered_gpo = cse_filtered_gpo;
- ret = process_security_settings_cse
(state, smb_uri, &allowed_sids, &allowed_size,
&denied_sids, &denied_size);
- if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE, "process_security_settings_cse
FAILED\n");
goto done;
- }
- state->allowed_sids = allowed_sids;
- state->allowed_size = allowed_size;
- state->denied_sids = denied_sids;
- state->denied_size = denied_size;
- ret = EOK;
- done:
- if (ret == EOK) {
tevent_req_done(req);
tevent_req_post(req, ev);
- } else {
tevent_req_error(req, ret);
tevent_req_post(req, ev);
- }
You can extract tevent_req_post() and run it unconditionally after the if block.
- return req;
+}
+int ad_gpo_process_cse_recv(struct tevent_req *req,
TALLOC_CTX *mem_ctx,
char ***allowed_sids,
int *allowed_size,
char ***denied_sids,
int *denied_size)
+{
- struct ad_gpo_process_cse_state *state =
tevent_req_data(req, struct ad_gpo_process_cse_state);
- TEVENT_REQ_RETURN_ON_ERROR(req);
- *allowed_sids = state->allowed_sids;
- *allowed_size = state->allowed_size;
I guess talloc_steal is missing here.
Both allowed_sids and denied_sids are populated using ini_get_string_config_array(), which does not use talloc memory allocation.
Regards, Yassir.
On Thu, Jun 12, 2014 at 04:37:25PM -0400, Yassir Elley wrote:
----- Original Message -----
On Fri, May 30, 2014 at 12:35:32PM -0400, Yassir Elley wrote:
----- Original Message -----
On (29/05/14 09:06), Yassir Elley wrote:
----- Original Message -----
...
I didn't had a chance to do functional testings yet, I'll send my results tomorrow. For a start please find my comments on the code inline.
Please let me know if you have additional comments after functional testing.
Basic functional tests were working fine, I was able to reject access and grant it again by modifying the GPO on the AD DC.
While watching the logs I've seen the smb uri smb://domain.name/sysvol/... A DNS query for domain.name will return the list of all DCs in domain.name and libsmbclient will probably pick the first which might be in a completely different location than the client. I would suggest to replace domain.name with the name of the DC SSSD is currently connected to.
As a general comment, I would prefer to wait until the file transfer is done in a child process before pushing this patch to master. Because otherwise imo the risk of blocking the backend for too long is too high.
bye, Sumit
...
I have two comments here. First, is there a way to avoid having deny rules? The reason is that we had some troubles in the past here, because when checking deny rules all group memberships of a user must be available to properly deny access. If some are missing access might be granted although it should be rejected. I guess it is not possible to get rid of them because they will be set on the AD side and we have to respect them.
I agree that relying on deny rules for security is problematic, b/c it can be difficult to exhaustively prove a negative. Don't we have the same challenge with the simple_access_provider's simple_deny_groups?
yes, but I guess there were request to add deny groups as well. But since this will be set in sssd.conf users are free to use it or not.
In any case, I also agree with your conclusion that we have to respect the GPO policy settings defined on the AD side.
Second, you are only checking SeInteractiveLogonRight and SeDenyInteractiveLogonRight. What about Se[Deny][Network|Batch|Service|RemoteInteractive]LogonRight? Are you planning to integrate them as well and relate them to different PAM services?
The plan for the initial deployment is to only support SeInteractiveLogonRight and SeDenyInteractiveLogonRight.
ok, but please keep in mind that it would be hard to add support for different login types, e.g. network vs. console, later on without breaking backward compatibility.
...
- ret = ini_get_config_valueobj(RIGHTS_SECTION, name, ini_config,
INI_GET_FIRST_VALUE, &vobj);
- if (vobj == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "section/name not found: [%s][%s]\n",
RIGHTS_SECTION, name);
return EOK;
- }
- sids = ini_get_string_config_array(vobj, NULL, &num_sids, &ret);
- if (ret) {
Please do explicit checks, in this case (ret != 0) because according to the docs ini_get_string_config_array() returns 0 on success.
Doesn't C define "true" as a non-zero value? Or are you suggesting this just for clarity?
For clarity, see e.g. Andreas' blog post http://blog.cryptomilk.org/2013/03/28/writing-and-reading-code/ .
I think we have the practice in SSSD to use 'if (a)' and 'if (!a)' only if 'a' is a bool or a pointer. Personally I prefer '!= NULL' and '== NULL' with pointers as well.
DEBUG(SSSDBG_CRIT_FAILURE,
"ini_get_string_config_array failed [%d][%s]\n", ret,
strerror(ret));
return ret;
- }
- for (i = 0; i <num_sids; i++) {
- if (sids[i][0] == '*') {
sids[i]++;
- }
- }
What is this loop good for?
The GptTmpl.inf policy files I have seen all include an asterisk before the user_sid or group_sid; this loop removes the asterisk so that the policy file sids can be correctly compared against the cached sids (which don't have asterisks).
Please add this as a comment for the loop.
...
+}
+int ad_gpo_process_cse_recv(struct tevent_req *req,
TALLOC_CTX *mem_ctx,
char ***allowed_sids,
int *allowed_size,
char ***denied_sids,
int *denied_size)
+{
- struct ad_gpo_process_cse_state *state =
tevent_req_data(req, struct ad_gpo_process_cse_state);
- TEVENT_REQ_RETURN_ON_ERROR(req);
- *allowed_sids = state->allowed_sids;
- *allowed_size = state->allowed_size;
I guess talloc_steal is missing here.
Both allowed_sids and denied_sids are populated using ini_get_string_config_array(), which does not use talloc memory allocation.
I see. I was confused by mem_ctx in the argument list. I would suggest to copy the lists to talloc arrays here or even at a lower level because it is confusing and the lists are currently not freed in ad_gpo_cse_done(). It would be even more difficult if you start to read the list from cache, because here talloc arrays would be used.
bye, Sumit
----- Original Message -----
On Thu, Jun 12, 2014 at 04:37:25PM -0400, Yassir Elley wrote:
----- Original Message -----
On Fri, May 30, 2014 at 12:35:32PM -0400, Yassir Elley wrote:
----- Original Message -----
On (29/05/14 09:06), Yassir Elley wrote:
----- Original Message -----
...
I didn't had a chance to do functional testings yet, I'll send my results tomorrow. For a start please find my comments on the code inline.
Please let me know if you have additional comments after functional testing.
Basic functional tests were working fine, I was able to reject access and grant it again by modifying the GPO on the AD DC.
While watching the logs I've seen the smb uri smb://domain.name/sysvol/... A DNS query for domain.name will return the list of all DCs in domain.name and libsmbclient will probably pick the first which might be in a completely different location than the client. I would suggest to replace domain.name with the name of the DC SSSD is currently connected to.
Is there an easy way to get the name of the DC that SSSD is currently connected to?
As a general comment, I would prefer to wait until the file transfer is done in a child process before pushing this patch to master. Because otherwise imo the risk of blocking the backend for too long is too high.
That's fine. I wanted to facilitate the review of the basic gpo-smb code, by not introducing the complexities of a child process. Now that this review is complete, I'll modify the code per the review comments, and then integrate the code into a gpo_child process patch.
bye, Sumit
...
I have two comments here. First, is there a way to avoid having deny rules? The reason is that we had some troubles in the past here, because when checking deny rules all group memberships of a user must be available to properly deny access. If some are missing access might be granted although it should be rejected. I guess it is not possible to get rid of them because they will be set on the AD side and we have to respect them.
I agree that relying on deny rules for security is problematic, b/c it can be difficult to exhaustively prove a negative. Don't we have the same challenge with the simple_access_provider's simple_deny_groups?
yes, but I guess there were request to add deny groups as well. But since this will be set in sssd.conf users are free to use it or not.
In theory, we could support just the Allow rules in the policy file (and not support the Deny rules), but I think this would be confusing for AD admins. We should support both Allow rules and Deny rules.
In any case, I also agree with your conclusion that we have to respect the GPO policy settings defined on the AD side.
Second, you are only checking SeInteractiveLogonRight and SeDenyInteractiveLogonRight. What about Se[Deny][Network|Batch|Service|RemoteInteractive]LogonRight? Are you planning to integrate them as well and relate them to different PAM services?
The plan for the initial deployment is to only support SeInteractiveLogonRight and SeDenyInteractiveLogonRight.
ok, but please keep in mind that it would be hard to add support for different login types, e.g. network vs. console, later on without breaking backward compatibility.
We are initially supporting console login as a feature. If we later decide to support network login as a feature (in a subsequent release), what is the concern regarding backward compatibility?
...
- ret = ini_get_config_valueobj(RIGHTS_SECTION, name, ini_config,
INI_GET_FIRST_VALUE, &vobj);
- if (vobj == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "section/name not found: [%s][%s]\n",
RIGHTS_SECTION, name);
return EOK;
- }
- sids = ini_get_string_config_array(vobj, NULL, &num_sids, &ret);
- if (ret) {
Please do explicit checks, in this case (ret != 0) because according to the docs ini_get_string_config_array() returns 0 on success.
Doesn't C define "true" as a non-zero value? Or are you suggesting this just for clarity?
For clarity, see e.g. Andreas' blog post http://blog.cryptomilk.org/2013/03/28/writing-and-reading-code/ .
I think we have the practice in SSSD to use 'if (a)' and 'if (!a)' only if 'a' is a bool or a pointer. Personally I prefer '!= NULL' and '== NULL' with pointers as well.
OK.
DEBUG(SSSDBG_CRIT_FAILURE,
"ini_get_string_config_array failed [%d][%s]\n", ret,
strerror(ret));
return ret;
- }
- for (i = 0; i <num_sids; i++) {
- if (sids[i][0] == '*') {
sids[i]++;
- }
- }
What is this loop good for?
The GptTmpl.inf policy files I have seen all include an asterisk before the user_sid or group_sid; this loop removes the asterisk so that the policy file sids can be correctly compared against the cached sids (which don't have asterisks).
Please add this as a comment for the loop.
OK.
...
+}
+int ad_gpo_process_cse_recv(struct tevent_req *req,
TALLOC_CTX *mem_ctx,
char ***allowed_sids,
int *allowed_size,
char ***denied_sids,
int *denied_size)
+{
- struct ad_gpo_process_cse_state *state =
tevent_req_data(req, struct ad_gpo_process_cse_state);
- TEVENT_REQ_RETURN_ON_ERROR(req);
- *allowed_sids = state->allowed_sids;
- *allowed_size = state->allowed_size;
I guess talloc_steal is missing here.
Both allowed_sids and denied_sids are populated using ini_get_string_config_array(), which does not use talloc memory allocation.
I see. I was confused by mem_ctx in the argument list. I would suggest to copy the lists to talloc arrays here or even at a lower level because it is confusing and the lists are currently not freed in ad_gpo_cse_done(). It would be even more difficult if you start to read the list from cache, because here talloc arrays would be used.
OK.
bye, Sumit _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Regards, Yassir.
----- Original Message -----
----- Original Message -----
On Thu, Jun 12, 2014 at 04:37:25PM -0400, Yassir Elley wrote:
----- Original Message -----
On Fri, May 30, 2014 at 12:35:32PM -0400, Yassir Elley wrote:
----- Original Message -----
On (29/05/14 09:06), Yassir Elley wrote: >----- Original Message -----
...
I didn't had a chance to do functional testings yet, I'll send my results tomorrow. For a start please find my comments on the code inline.
Please let me know if you have additional comments after functional testing.
Basic functional tests were working fine, I was able to reject access and grant it again by modifying the GPO on the AD DC.
While watching the logs I've seen the smb uri smb://domain.name/sysvol/... A DNS query for domain.name will return the list of all DCs in domain.name and libsmbclient will probably pick the first which might be in a completely different location than the client. I would suggest to replace domain.name with the name of the DC SSSD is currently connected to.
Is there an easy way to get the name of the DC that SSSD is currently connected to?
As a general comment, I would prefer to wait until the file transfer is done in a child process before pushing this patch to master. Because otherwise imo the risk of blocking the backend for too long is too high.
That's fine. I wanted to facilitate the review of the basic gpo-smb code, by not introducing the complexities of a child process. Now that this review is complete, I'll modify the code per the review comments, and then integrate the code into a gpo_child process patch.
bye, Sumit
...
I have two comments here. First, is there a way to avoid having deny rules? The reason is that we had some troubles in the past here, because when checking deny rules all group memberships of a user must be available to properly deny access. If some are missing access might be granted although it should be rejected. I guess it is not possible to get rid of them because they will be set on the AD side and we have to respect them.
I agree that relying on deny rules for security is problematic, b/c it can be difficult to exhaustively prove a negative. Don't we have the same challenge with the simple_access_provider's simple_deny_groups?
yes, but I guess there were request to add deny groups as well. But since this will be set in sssd.conf users are free to use it or not.
In theory, we could support just the Allow rules in the policy file (and not support the Deny rules), but I think this would be confusing for AD admins. We should support both Allow rules and Deny rules.
In any case, I also agree with your conclusion that we have to respect the GPO policy settings defined on the AD side.
Second, you are only checking SeInteractiveLogonRight and SeDenyInteractiveLogonRight. What about Se[Deny][Network|Batch|Service|RemoteInteractive]LogonRight? Are you planning to integrate them as well and relate them to different PAM services?
The plan for the initial deployment is to only support SeInteractiveLogonRight and SeDenyInteractiveLogonRight.
ok, but please keep in mind that it would be hard to add support for different login types, e.g. network vs. console, later on without breaking backward compatibility.
We are initially supporting console login as a feature. If we later decide to support network login as a feature (in a subsequent release), what is the concern regarding backward compatibility?
...
- ret = ini_get_config_valueobj(RIGHTS_SECTION, name, ini_config,
INI_GET_FIRST_VALUE, &vobj);
- if (vobj == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "section/name not found:
[%s][%s]\n",
RIGHTS_SECTION, name);
return EOK;
- }
- sids = ini_get_string_config_array(vobj, NULL, &num_sids, &ret);
- if (ret) {
Please do explicit checks, in this case (ret != 0) because according to the docs ini_get_string_config_array() returns 0 on success.
Doesn't C define "true" as a non-zero value? Or are you suggesting this just for clarity?
For clarity, see e.g. Andreas' blog post http://blog.cryptomilk.org/2013/03/28/writing-and-reading-code/ .
I think we have the practice in SSSD to use 'if (a)' and 'if (!a)' only if 'a' is a bool or a pointer. Personally I prefer '!= NULL' and '== NULL' with pointers as well.
OK.
DEBUG(SSSDBG_CRIT_FAILURE,
"ini_get_string_config_array failed [%d][%s]\n", ret,
strerror(ret));
return ret;
- }
- for (i = 0; i <num_sids; i++) {
- if (sids[i][0] == '*') {
sids[i]++;
- }
- }
What is this loop good for?
The GptTmpl.inf policy files I have seen all include an asterisk before the user_sid or group_sid; this loop removes the asterisk so that the policy file sids can be correctly compared against the cached sids (which don't have asterisks).
Please add this as a comment for the loop.
OK.
...
+}
+int ad_gpo_process_cse_recv(struct tevent_req *req,
TALLOC_CTX *mem_ctx,
char ***allowed_sids,
int *allowed_size,
char ***denied_sids,
int *denied_size)
+{
- struct ad_gpo_process_cse_state *state =
tevent_req_data(req, struct ad_gpo_process_cse_state);
- TEVENT_REQ_RETURN_ON_ERROR(req);
- *allowed_sids = state->allowed_sids;
- *allowed_size = state->allowed_size;
I guess talloc_steal is missing here.
Both allowed_sids and denied_sids are populated using ini_get_string_config_array(), which does not use talloc memory allocation.
I see. I was confused by mem_ctx in the argument list. I would suggest to copy the lists to talloc arrays here or even at a lower level because it is confusing and the lists are currently not freed in ad_gpo_cse_done(). It would be even more difficult if you start to read the list from cache, because here talloc arrays would be used.
OK.
bye, Sumit _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Regards, Yassir. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
I have attached a patch, which integrates the gpo-smb code into a gpo_child process. You may have additional code review comments on the gpo_child part of the code, but I think I have addressed all of the initial gpo-smb code review comments in this patch, with the following two exceptions (for which I need some more guidance from you): * You suggested adding the new minimal version (libini_config-devel >= 1.1) to the configure check. Can you clarify what you mean by this, or preferably provide a diff? We probably only want to enforce this minimal version configure check for the case in which the gpo code is being used, right? For example, if the ipa-provider is being used, then we wouldn't want this configure check. * You suggested using the name of the DC that SSSD is currently connected to in the smb uri (rather than the domain.name, which will require libsmbclient to perform a DNS resolution). Is there an easy way to get the name of the DC that SSSD is currently connected to? I am having trouble finding it.
Regards, Yassir.
On (15/06/14 19:08), Yassir Elley wrote:
----- Original Message -----
----- Original Message -----
On Thu, Jun 12, 2014 at 04:37:25PM -0400, Yassir Elley wrote:
----- Original Message -----
On Fri, May 30, 2014 at 12:35:32PM -0400, Yassir Elley wrote:
----- Original Message ----- > On (29/05/14 09:06), Yassir Elley wrote: > >----- Original Message -----
...
I didn't had a chance to do functional testings yet, I'll send my results tomorrow. For a start please find my comments on the code inline.
Please let me know if you have additional comments after functional testing.
Basic functional tests were working fine, I was able to reject access and grant it again by modifying the GPO on the AD DC.
While watching the logs I've seen the smb uri smb://domain.name/sysvol/... A DNS query for domain.name will return the list of all DCs in domain.name and libsmbclient will probably pick the first which might be in a completely different location than the client. I would suggest to replace domain.name with the name of the DC SSSD is currently connected to.
Is there an easy way to get the name of the DC that SSSD is currently connected to?
As a general comment, I would prefer to wait until the file transfer is done in a child process before pushing this patch to master. Because otherwise imo the risk of blocking the backend for too long is too high.
That's fine. I wanted to facilitate the review of the basic gpo-smb code, by not introducing the complexities of a child process. Now that this review is complete, I'll modify the code per the review comments, and then integrate the code into a gpo_child process patch.
bye, Sumit
...
I have two comments here. First, is there a way to avoid having deny rules? The reason is that we had some troubles in the past here, because when checking deny rules all group memberships of a user must be available to properly deny access. If some are missing access might be granted although it should be rejected. I guess it is not possible to get rid of them because they will be set on the AD side and we have to respect them.
I agree that relying on deny rules for security is problematic, b/c it can be difficult to exhaustively prove a negative. Don't we have the same challenge with the simple_access_provider's simple_deny_groups?
yes, but I guess there were request to add deny groups as well. But since this will be set in sssd.conf users are free to use it or not.
In theory, we could support just the Allow rules in the policy file (and not support the Deny rules), but I think this would be confusing for AD admins. We should support both Allow rules and Deny rules.
In any case, I also agree with your conclusion that we have to respect the GPO policy settings defined on the AD side.
Second, you are only checking SeInteractiveLogonRight and SeDenyInteractiveLogonRight. What about Se[Deny][Network|Batch|Service|RemoteInteractive]LogonRight? Are you planning to integrate them as well and relate them to different PAM services?
The plan for the initial deployment is to only support SeInteractiveLogonRight and SeDenyInteractiveLogonRight.
ok, but please keep in mind that it would be hard to add support for different login types, e.g. network vs. console, later on without breaking backward compatibility.
We are initially supporting console login as a feature. If we later decide to support network login as a feature (in a subsequent release), what is the concern regarding backward compatibility?
...
- ret = ini_get_config_valueobj(RIGHTS_SECTION, name, ini_config,
INI_GET_FIRST_VALUE, &vobj);
- if (vobj == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "section/name not found:
[%s][%s]\n",
RIGHTS_SECTION, name);
return EOK;
- }
- sids = ini_get_string_config_array(vobj, NULL, &num_sids, &ret);
- if (ret) {
Please do explicit checks, in this case (ret != 0) because according to the docs ini_get_string_config_array() returns 0 on success.
Doesn't C define "true" as a non-zero value? Or are you suggesting this just for clarity?
For clarity, see e.g. Andreas' blog post http://blog.cryptomilk.org/2013/03/28/writing-and-reading-code/ .
I think we have the practice in SSSD to use 'if (a)' and 'if (!a)' only if 'a' is a bool or a pointer. Personally I prefer '!= NULL' and '== NULL' with pointers as well.
OK.
DEBUG(SSSDBG_CRIT_FAILURE,
"ini_get_string_config_array failed [%d][%s]\n", ret,
strerror(ret));
return ret;
- }
- for (i = 0; i <num_sids; i++) {
- if (sids[i][0] == '*') {
sids[i]++;
- }
- }
What is this loop good for?
The GptTmpl.inf policy files I have seen all include an asterisk before the user_sid or group_sid; this loop removes the asterisk so that the policy file sids can be correctly compared against the cached sids (which don't have asterisks).
Please add this as a comment for the loop.
OK.
...
+}
+int ad_gpo_process_cse_recv(struct tevent_req *req,
TALLOC_CTX *mem_ctx,
char ***allowed_sids,
int *allowed_size,
char ***denied_sids,
int *denied_size)
+{
- struct ad_gpo_process_cse_state *state =
tevent_req_data(req, struct ad_gpo_process_cse_state);
- TEVENT_REQ_RETURN_ON_ERROR(req);
- *allowed_sids = state->allowed_sids;
- *allowed_size = state->allowed_size;
I guess talloc_steal is missing here.
Both allowed_sids and denied_sids are populated using ini_get_string_config_array(), which does not use talloc memory allocation.
I see. I was confused by mem_ctx in the argument list. I would suggest to copy the lists to talloc arrays here or even at a lower level because it is confusing and the lists are currently not freed in ad_gpo_cse_done(). It would be even more difficult if you start to read the list from cache, because here talloc arrays would be used.
OK.
bye, Sumit
Regards, Yassir.
I have attached a patch, which integrates the gpo-smb code into a gpo_child process. You may have additional code review comments on the gpo_child part of the code, but I think I have addressed all of the initial gpo-smb code review comments in this patch, with the following two exceptions (for which I need some more guidance from you):
- You suggested adding the new minimal version (libini_config-devel >= 1.1) to the configure check. Can you clarify what you mean by this, or preferably provide a diff? We probably only want to enforce this minimal version configure check for the case in which the gpo code is being used, right? For example, if the ipa-provider is being used, then we wouldn't want this configure check.
I think Sumit meant PKG_CHECK_MODULES(INI_CONFIG, [ini_config >= 1.1]]) in file src/external/libini_config.m4
ipa-provider and ad-provider are related due to ipa-server mode. On the other side ldap-provider can work with older version of libini_config. If you want to solve problem with your proposal you can add this autoconf test into src/external/samba.m4.
ding-libs is a simple library without complex dependencies. So I don't know if we want to complicate it but Sumit can have different preferences.
- You suggested using the name of the DC that SSSD is currently connected to in the smb uri (rather than the domain.name, which will require libsmbclient to perform a DNS resolution). Is there an easy way to get the name of the DC that SSSD is currently connected to? I am having trouble finding it.
Regards, Yassir.
From 5567854a940cee094a832bcae7271faa81b63bd0 Mon Sep 17 00:00:00 2001 From: Yassir Elley yelley@redhat.com Date: Wed, 21 May 2014 15:11:36 -0400 Subject: [PATCH] AD-GPO: Add gpo-smb implementation in gpo_child process
Makefile.am | 20 + contrib/sssd.spec.in | 2 +- src/providers/ad/ad_gpo.c | 822 +++++++++++++++++++++++++++++++++++++++- src/providers/ad/ad_gpo_child.c | 620 ++++++++++++++++++++++++++++++ 4 files changed, 1458 insertions(+), 6 deletions(-) create mode 100644 src/providers/ad/ad_gpo_child.c
diff --git a/Makefile.am b/Makefile.am index f64c77a5bdd7f53b41bd37572b2d91434a9893a6..4c2f13b1f9d6631cde82d8e61b89eecf4d2fbec5 100644 --- a/Makefile.am +++ b/Makefile.am @@ -107,6 +107,7 @@ sssdlibexec_PROGRAMS = \ sssd_be \ krb5_child \ ldap_child \
- gpo_child \ proxy_child
if BUILD_SUDO sssdlibexec_PROGRAMS += sssd_sudo @@ -2294,6 +2295,25 @@ ldap_child_LDADD = \ $(DHASH_LIBS) \ $(KRB5_LIBS)
It would be better if you could build gpo_child conditionally.
if BUILD_SAMBA sssdlibexec_PROGRAMS += gpo_child endif
+gpo_child_SOURCES = \
- src/providers/ad/ad_gpo_child.c \
- src/util/atomic_io.c \
- src/util/util.c \
- src/util/signal.c
+gpo_child_CFLAGS = \
- $(AM_CFLAGS) \
- $(POPT_CFLAGS) \
- $(KRB5_CFLAGS) \
- $(INI_CONFIG_CFLAGS) \
- $(SMBCLIENT_CFLAGS)
+gpo_child_LDADD = \
- libsss_debug.la \
- $(TALLOC_LIBS) \
- $(POPT_LIBS) \
- $(DHASH_LIBS) \
- $(INI_CONFIG_LIBS) \
- $(SMBCLIENT_LIBS)
proxy_child_SOURCES = \ src/providers/proxy/proxy_child.c \ src/providers/data_provider_iface_generated.c \ diff --git a/contrib/sssd.spec.in b/contrib/sssd.spec.in index b5e7903713ccc6944fbed4f84c2f0413a257b760..10704ae48cf5636de2a5cbbb42fc00b99052144f 100644 --- a/contrib/sssd.spec.in +++ b/contrib/sssd.spec.in @@ -97,7 +97,7 @@ BuildRequires: libtdb-devel BuildRequires: libldb-devel BuildRequires: libdhash-devel >= 0.4.2 BuildRequires: libcollection-devel -BuildRequires: libini_config-devel +BuildRequires: libini_config-devel >= 1.1 BuildRequires: dbus-devel BuildRequires: dbus-libs %if 0%{?rhel5_minor} >= 7 diff --git a/src/providers/ad/ad_gpo.c b/src/providers/ad/ad_gpo.c index bb6896e70ae96036b1a9e58a6c8e25d3d8f1543f..78c2a7da0352e0ea464f01751b907dbc4e02e861 100644 --- a/src/providers/ad/ad_gpo.c +++ b/src/providers/ad/ad_gpo.c
//snip
+int ad_gpo_process_cse_recv(struct tevent_req *req,
TALLOC_CTX *mem_ctx,
int *_allowed_size,
char ***_allowed_sids,
int *_denied_size,
char ***_denied_sids)
+{
- char *smb_uri;
^^^^^^^^^^^^^^ unused variable
- int ret;
- char **allowed_sids;
- int allowed_size;
- char **denied_sids;
- int denied_size;
- struct ad_gpo_process_cse_state *state;
- state = tevent_req_data(req, struct ad_gpo_process_cse_state);
- TEVENT_REQ_RETURN_ON_ERROR(req);
- ret = parse_gpo_child_response(mem_ctx, state->buf, state->len,
&allowed_sids,
&allowed_size,
&denied_sids,
&denied_size);
- if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE,
"Cannot parse child response: [%d][%s]\n", ret, strerror(ret));
return ret;
- }
- *_allowed_size = allowed_size;
- *_allowed_sids = talloc_steal(mem_ctx, allowed_sids);
- *_denied_size = denied_size;
- *_denied_sids = talloc_steal(mem_ctx, denied_sids);
- return EOK;
+}
+static errno_t +gpo_fork_child(struct tevent_req *req) +{
- int pipefd_to_child[2];
- int pipefd_from_child[2];
- pid_t pid;
- int ret;
- errno_t err;
- struct ad_gpo_process_cse_state *state;
- state = tevent_req_data(req, struct ad_gpo_process_cse_state);
- ret = pipe(pipefd_from_child);
- if (ret == -1) {
err = errno;
DEBUG(SSSDBG_CRIT_FAILURE,
"pipe failed [%d][%s].\n", errno, strerror(errno));
return err;
- }
- ret = pipe(pipefd_to_child);
- if (ret == -1) {
err = errno;
DEBUG(SSSDBG_CRIT_FAILURE,
"pipe failed [%d][%s].\n", errno, strerror(errno));
return err;
- }
- pid = fork();
- if (pid == 0) { /* child */
err = exec_child(state,
pipefd_to_child, pipefd_from_child,
GPO_CHILD, gpo_child_debug_fd);
DEBUG(SSSDBG_CRIT_FAILURE, "Could not exec gpo_child: [%d][%s].\n",
err, strerror(err));
return err;
- } else if (pid > 0) { /* parent */
state->child_pid = pid;
state->io->read_from_child_fd = pipefd_from_child[0];
close(pipefd_from_child[1]);
state->io->write_to_child_fd = pipefd_to_child[1];
close(pipefd_to_child[0]);
fd_nonblocking(state->io->read_from_child_fd);
fd_nonblocking(state->io->write_to_child_fd);
ret = child_handler_setup(state->ev, pid, NULL, NULL, NULL);
if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE,
"Could not set up child signal handler\n");
return ret;
}
- } else { /* error */
err = errno;
DEBUG(SSSDBG_CRIT_FAILURE,
"fork failed [%d][%s].\n", errno, strerror(errno));
return err;
- }
- return EOK;
+} diff --git a/src/providers/ad/ad_gpo_child.c b/src/providers/ad/ad_gpo_child.c new file mode 100644 index 0000000000000000000000000000000000000000..4659d8c3666841ea7bd84194cc1fdcfb671851ed --- /dev/null +++ b/src/providers/ad/ad_gpo_child.c @@ -0,0 +1,620 @@ +/*
- SSSD
- AD GPO Backend Module -- perform SMB and CSE processing in a child process
- Authors:
Yassir Elley <yelley@redhat.com>
- Copyright (C) 2013 Red Hat
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
- the Free Software Foundation; either version 3 of the License, or
- (at your option) any later version.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program. If not, see http://www.gnu.org/licenses/.
+*/
//snip
+static errno_t +process_security_settings_cse(TALLOC_CTX *mem_ctx,
const char *smb_uri,
char ***_allowed_sids,
int *_allowed_size,
char ***_denied_sids,
int *_denied_size)
+{
- SMBCCTX *context;
- int ret = 0;
- uint8_t *buf = NULL;
- int bytesread = 0;
- char **allowed_sids;
- char **denied_sids;
- int allowed_size = 0;
- int denied_size = 0;
- TALLOC_CTX *tmp_ctx = NULL;
- tmp_ctx = talloc_new(NULL);
- if (tmp_ctx == NULL) {
ret = ENOMEM;
goto done;
You should immediatelly return ENOMEM, otherwise context will be used unitialized in done section.
- }
- DEBUG(SSSDBG_TRACE_ALL, "%s\n", smb_uri);
- context = smbc_new_context();
- if (context == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "Could not allocate new smbc context\n");
ret = ENOMEM;
goto done;
- }
- smbc_setFunctionAuthData(context, sssd_krb_get_auth_data_fn);
- smbc_setOptionUseKerberos(context, 1);
- /* Initialize the context using the previously specified options */
- if (smbc_init_context(context) == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "Could not initialize smbc context\n");
ret = ENOMEM;
goto done;
- }
- /* Tell the compatibility layer to use this context */
- smbc_set_context(context);
- int remotehandle = smbc_open(smb_uri, O_RDONLY, 0755);
- if (remotehandle < 0) {
DEBUG(SSSDBG_CRIT_FAILURE, "smbc_open failed\n");
ret = EPIPE;
goto done;
- }
- buf = talloc_array(tmp_ctx, uint8_t, SMB_BUFFER_SIZE);
- bytesread = smbc_read(remotehandle, buf, SMB_BUFFER_SIZE);
- if(bytesread < 0) {
DEBUG(SSSDBG_CRIT_FAILURE, "smbc_read failed\n");
ret = EPIPE;
goto done;
- }
- DEBUG(SSSDBG_CRIT_FAILURE, "bytesread: %d\n", bytesread);
- smbc_close(remotehandle);
- ret = ad_gpo_parse_security_cse_buffer(tmp_ctx,
buf,
bytesread,
&allowed_sids,
&allowed_size,
&denied_sids,
&denied_size);
Function ad_gpo_parse_security_cse_buffer can fail in many cases ENOMEM, problem with initialising ini_config_* and output variable will be uninitialized in next lines.
- /* TBD: allowed/denied_sids/size should be stored in cache */
- *_allowed_sids = talloc_steal(mem_ctx, allowed_sids);
- *_allowed_size = allowed_size;
- *_denied_sids = talloc_steal(mem_ctx, denied_sids);
- *_denied_size = denied_size;
- done:
- smbc_free_context(context, 0);
- talloc_free(tmp_ctx);
- return ret;
+}
+int +main(int argc, const char *argv[]) +{
- int opt;
- poptContext pc;
- int debug_fd = -1;
- errno_t ret;
- int result;
- TALLOC_CTX *main_ctx = NULL;
- uint8_t *buf = NULL;
- ssize_t len = 0;
- struct input_buffer *ibuf = NULL;
- struct response *resp = NULL;
- size_t written;
- char **allowed_sids;
- int allowed_size;
- char **denied_sids;
- int denied_size;
- int j;
- struct poptOption long_options[] = {
POPT_AUTOHELP
{"debug-level", 'd', POPT_ARG_INT, &debug_level, 0,
_("Debug level"), NULL},
{"debug-timestamps", 0, POPT_ARG_INT, &debug_timestamps, 0,
_("Add debug timestamps"), NULL},
{"debug-microseconds", 0, POPT_ARG_INT, &debug_microseconds, 0,
_("Show timestamps with microseconds"), NULL},
{"debug-fd", 0, POPT_ARG_INT, &debug_fd, 0,
_("An open file descriptor for the debug logs"), NULL},
POPT_TABLEEND
- };
- /* Set debug level to invalid value so we can decide if -d 0 was used. */
- debug_level = SSSDBG_INVALID;
- pc = poptGetContext(argv[0], argc, argv, long_options, 0);
- while((opt = poptGetNextOpt(pc)) != -1) {
switch(opt) {
default:
fprintf(stderr, "\nInvalid option %s: %s\n\n",
poptBadOption(pc, 0), poptStrerror(opt));
poptPrintUsage(pc, stderr, 0);
_exit(-1);
}
- }
- poptFreeContext(pc);
- DEBUG_INIT(debug_level);
- debug_prg_name = talloc_asprintf(NULL, "[sssd[gpo_child[%d]]]", getpid());
- if (debug_prg_name == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "talloc_asprintf failed.\n");
ret = ENOMEM;
ret is not used in fail. You can ammend previous debug message if you want.
goto fail;
- }
- if (debug_fd != -1) {
ret = set_debug_file_from_fd(debug_fd);
if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE, "set_debug_file_from_fd failed.\n");
}
- }
- DEBUG(SSSDBG_TRACE_FUNC, "gpo_child started.\n");
- main_ctx = talloc_new(NULL);
- if (main_ctx == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new failed.\n");
talloc_free(discard_const(debug_prg_name));
goto fail;
- }
- talloc_steal(main_ctx, debug_prg_name);
- buf = talloc_size(main_ctx, sizeof(uint8_t)*IN_BUF_SIZE);
- if (buf == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "talloc_size failed.\n");
goto fail;
- }
- ibuf = talloc_zero(main_ctx, struct input_buffer);
- if (ibuf == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "talloc_zero failed.\n");
goto fail;
- }
- DEBUG(SSSDBG_TRACE_FUNC, "context initialized\n");
- errno = 0;
- len = sss_atomic_read_s(STDIN_FILENO, buf, IN_BUF_SIZE);
- if (len == -1) {
ret = errno;
DEBUG(SSSDBG_CRIT_FAILURE, "read failed [%d][%s].\n", ret, strerror(ret));
goto fail;
- }
- close(STDIN_FILENO);
- ret = unpack_buffer(buf, len, ibuf);
- if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE,
"unpack_buffer failed.[%d][%s].\n", ret, strerror(ret));
goto fail;
- }
- DEBUG(SSSDBG_TRACE_FUNC, "processing security settings\n");
- result = process_security_settings_cse(main_ctx,
ibuf->smb_uri,
&allowed_sids,
&allowed_size,
&denied_sids,
&denied_size);
You should test result of function process_security_settings_cse. It may happen that output variables will not be initialised.
- DEBUG(SSSDBG_CRIT_FAILURE, "allowed_size = %d\n", allowed_size);
- for (j= 0; j < allowed_size; j++) {
DEBUG(SSSDBG_CRIT_FAILURE, "allowed_sids[%d] = %s\n", j,
allowed_sids[j]);
- }
- DEBUG(SSSDBG_CRIT_FAILURE, "denied_size = %d\n", denied_size);
- for (j= 0; j < denied_size; j++) {
DEBUG(SSSDBG_CRIT_FAILURE, " denied_sids[%d] = %s\n", j,
denied_sids[j]);
- }
- result = EOK;
Is there a reason for this assignment? Will it be solved with my previous comment?
- ret = prepare_response(main_ctx, result, allowed_size, allowed_sids, denied_size, denied_sids, &resp);
- if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE, "prepare_response failed. [%d][%s].\n",
ret, strerror(ret));
goto fail;
- }
- errno = 0;
- DEBUG(SSSDBG_TRACE_FUNC, "resp->size: %zu\n", resp->size);
- written = sss_atomic_write_s(STDOUT_FILENO, resp->buf, resp->size);
- if (written == -1) {
ret = errno;
DEBUG(SSSDBG_CRIT_FAILURE, "write failed [%d][%s].\n", ret,
strerror(ret));
goto fail;
- }
- if (written != resp->size) {
DEBUG(SSSDBG_CRIT_FAILURE, "Expected to write %zu bytes, wrote %zu\n",
resp->size, written);
goto fail;
- }
- DEBUG(SSSDBG_TRACE_FUNC, "gpo_child completed successfully\n");
- close(STDOUT_FILENO);
- talloc_free(main_ctx);
- _exit(0);
It will be better to use constant EXIT_SUCCESS
+fail:
- DEBUG(SSSDBG_CRIT_FAILURE, "gpo_child failed!\n");
- close(STDOUT_FILENO);
- talloc_free(main_ctx);
- _exit(-1);
and here EXIT_FAILURE
LS
----- Original Message -----
On (15/06/14 19:08), Yassir Elley wrote:
----- Original Message -----
----- Original Message -----
On Thu, Jun 12, 2014 at 04:37:25PM -0400, Yassir Elley wrote:
----- Original Message -----
On Fri, May 30, 2014 at 12:35:32PM -0400, Yassir Elley wrote: > > > ----- Original Message ----- > > On (29/05/14 09:06), Yassir Elley wrote: > > >----- Original Message ----- >
...
I didn't had a chance to do functional testings yet, I'll send my results tomorrow. For a start please find my comments on the code inline.
Please let me know if you have additional comments after functional testing.
Basic functional tests were working fine, I was able to reject access and grant it again by modifying the GPO on the AD DC.
While watching the logs I've seen the smb uri smb://domain.name/sysvol/... A DNS query for domain.name will return the list of all DCs in domain.name and libsmbclient will probably pick the first which might be in a completely different location than the client. I would suggest to replace domain.name with the name of the DC SSSD is currently connected to.
Is there an easy way to get the name of the DC that SSSD is currently connected to?
As a general comment, I would prefer to wait until the file transfer is done in a child process before pushing this patch to master. Because otherwise imo the risk of blocking the backend for too long is too high.
That's fine. I wanted to facilitate the review of the basic gpo-smb code, by not introducing the complexities of a child process. Now that this review is complete, I'll modify the code per the review comments, and then integrate the code into a gpo_child process patch.
bye, Sumit
...
I have two comments here. First, is there a way to avoid having deny rules? The reason is that we had some troubles in the past here, because when checking deny rules all group memberships of a user must be available to properly deny access. If some are missing access might be granted although it should be rejected. I guess it is not possible to get rid of them because they will be set on the AD side and we have to respect them.
I agree that relying on deny rules for security is problematic, b/c it can be difficult to exhaustively prove a negative. Don't we have the same challenge with the simple_access_provider's simple_deny_groups?
yes, but I guess there were request to add deny groups as well. But since this will be set in sssd.conf users are free to use it or not.
In theory, we could support just the Allow rules in the policy file (and not support the Deny rules), but I think this would be confusing for AD admins. We should support both Allow rules and Deny rules.
In any case, I also agree with your conclusion that we have to respect the GPO policy settings defined on the AD side.
Second, you are only checking SeInteractiveLogonRight and SeDenyInteractiveLogonRight. What about Se[Deny][Network|Batch|Service|RemoteInteractive]LogonRight? Are you planning to integrate them as well and relate them to different PAM services?
The plan for the initial deployment is to only support SeInteractiveLogonRight and SeDenyInteractiveLogonRight.
ok, but please keep in mind that it would be hard to add support for different login types, e.g. network vs. console, later on without breaking backward compatibility.
We are initially supporting console login as a feature. If we later decide to support network login as a feature (in a subsequent release), what is the concern regarding backward compatibility?
...
> + > + ret = ini_get_config_valueobj(RIGHTS_SECTION, name, ini_config, > + INI_GET_FIRST_VALUE, &vobj); > + if (vobj == NULL) { > + DEBUG(SSSDBG_CRIT_FAILURE, "section/name not found: > [%s][%s]\n", > + RIGHTS_SECTION, name); > + return EOK; > + } > + sids = ini_get_string_config_array(vobj, NULL, &num_sids, > &ret); > + > + if (ret) {
Please do explicit checks, in this case (ret != 0) because according to the docs ini_get_string_config_array() returns 0 on success.
Doesn't C define "true" as a non-zero value? Or are you suggesting this just for clarity?
For clarity, see e.g. Andreas' blog post http://blog.cryptomilk.org/2013/03/28/writing-and-reading-code/ .
I think we have the practice in SSSD to use 'if (a)' and 'if (!a)' only if 'a' is a bool or a pointer. Personally I prefer '!= NULL' and '== NULL' with pointers as well.
OK.
> + DEBUG(SSSDBG_CRIT_FAILURE, > + "ini_get_string_config_array failed [%d][%s]\n", ret, > strerror(ret)); > + return ret; > + } > + > + for (i = 0; i <num_sids; i++) { > + if (sids[i][0] == '*') { > + sids[i]++; > + } > + }
What is this loop good for?
The GptTmpl.inf policy files I have seen all include an asterisk before the user_sid or group_sid; this loop removes the asterisk so that the policy file sids can be correctly compared against the cached sids (which don't have asterisks).
Please add this as a comment for the loop.
OK.
...
> +} > + > +int ad_gpo_process_cse_recv(struct tevent_req *req, > + TALLOC_CTX *mem_ctx, > + char ***allowed_sids, > + int *allowed_size, > + char ***denied_sids, > + int *denied_size) > +{ > + struct ad_gpo_process_cse_state *state = > + tevent_req_data(req, struct ad_gpo_process_cse_state); > + > + TEVENT_REQ_RETURN_ON_ERROR(req); > + > + *allowed_sids = state->allowed_sids; > + *allowed_size = state->allowed_size;
I guess talloc_steal is missing here.
Both allowed_sids and denied_sids are populated using ini_get_string_config_array(), which does not use talloc memory allocation.
I see. I was confused by mem_ctx in the argument list. I would suggest to copy the lists to talloc arrays here or even at a lower level because it is confusing and the lists are currently not freed in ad_gpo_cse_done(). It would be even more difficult if you start to read the list from cache, because here talloc arrays would be used.
OK.
bye, Sumit
Regards, Yassir.
I have attached a patch, which integrates the gpo-smb code into a gpo_child process. You may have additional code review comments on the gpo_child part of the code, but I think I have addressed all of the initial gpo-smb code review comments in this patch, with the following two exceptions (for which I need some more guidance from you):
- You suggested adding the new minimal version (libini_config-devel >= 1.1)
to the configure check. Can you clarify what you mean by this, or preferably provide a diff? We probably only want to enforce this minimal version configure check for the case in which the gpo code is being used, right? For example, if the ipa-provider is being used, then we wouldn't want this configure check.
I think Sumit meant PKG_CHECK_MODULES(INI_CONFIG, [ini_config >= 1.1]]) in file src/external/libini_config.m4
ipa-provider and ad-provider are related due to ipa-server mode. On the other side ldap-provider can work with older version of libini_config. If you want to solve problem with your proposal you can add this autoconf test into src/external/samba.m4.
ding-libs is a simple library without complex dependencies. So I don't know if we want to complicate it but Sumit can have different preferences.
Since the new version of libini_config is only needed when libsmbclient is being used, I have added the configure check to samba.m4.
- You suggested using the name of the DC that SSSD is currently connected to
in the smb uri (rather than the domain.name, which will require libsmbclient to perform a DNS resolution). Is there an easy way to get the name of the DC that SSSD is currently connected to? I am having trouble finding it.
Regards, Yassir.
From 5567854a940cee094a832bcae7271faa81b63bd0 Mon Sep 17 00:00:00 2001 From: Yassir Elley yelley@redhat.com Date: Wed, 21 May 2014 15:11:36 -0400 Subject: [PATCH] AD-GPO: Add gpo-smb implementation in gpo_child process
Makefile.am | 20 + contrib/sssd.spec.in | 2 +- src/providers/ad/ad_gpo.c | 822 +++++++++++++++++++++++++++++++++++++++- src/providers/ad/ad_gpo_child.c | 620 ++++++++++++++++++++++++++++++ 4 files changed, 1458 insertions(+), 6 deletions(-) create mode 100644 src/providers/ad/ad_gpo_child.c
diff --git a/Makefile.am b/Makefile.am index f64c77a5bdd7f53b41bd37572b2d91434a9893a6..4c2f13b1f9d6631cde82d8e61b89eecf4d2fbec5 100644 --- a/Makefile.am +++ b/Makefile.am @@ -107,6 +107,7 @@ sssdlibexec_PROGRAMS = \ sssd_be \ krb5_child \ ldap_child \
- gpo_child \ proxy_child
if BUILD_SUDO sssdlibexec_PROGRAMS += sssd_sudo @@ -2294,6 +2295,25 @@ ldap_child_LDADD = \ $(DHASH_LIBS) \ $(KRB5_LIBS)
It would be better if you could build gpo_child conditionally.
if BUILD_SAMBA sssdlibexec_PROGRAMS += gpo_child endif
+gpo_child_SOURCES = \
- src/providers/ad/ad_gpo_child.c \
- src/util/atomic_io.c \
- src/util/util.c \
- src/util/signal.c
+gpo_child_CFLAGS = \
- $(AM_CFLAGS) \
- $(POPT_CFLAGS) \
- $(KRB5_CFLAGS) \
- $(INI_CONFIG_CFLAGS) \
- $(SMBCLIENT_CFLAGS)
+gpo_child_LDADD = \
- libsss_debug.la \
- $(TALLOC_LIBS) \
- $(POPT_LIBS) \
- $(DHASH_LIBS) \
- $(INI_CONFIG_LIBS) \
- $(SMBCLIENT_LIBS)
proxy_child_SOURCES = \ src/providers/proxy/proxy_child.c \ src/providers/data_provider_iface_generated.c \ diff --git a/contrib/sssd.spec.in b/contrib/sssd.spec.in index b5e7903713ccc6944fbed4f84c2f0413a257b760..10704ae48cf5636de2a5cbbb42fc00b99052144f 100644 --- a/contrib/sssd.spec.in +++ b/contrib/sssd.spec.in @@ -97,7 +97,7 @@ BuildRequires: libtdb-devel BuildRequires: libldb-devel BuildRequires: libdhash-devel >= 0.4.2 BuildRequires: libcollection-devel -BuildRequires: libini_config-devel +BuildRequires: libini_config-devel >= 1.1 BuildRequires: dbus-devel BuildRequires: dbus-libs %if 0%{?rhel5_minor} >= 7 diff --git a/src/providers/ad/ad_gpo.c b/src/providers/ad/ad_gpo.c index bb6896e70ae96036b1a9e58a6c8e25d3d8f1543f..78c2a7da0352e0ea464f01751b907dbc4e02e861 100644 --- a/src/providers/ad/ad_gpo.c +++ b/src/providers/ad/ad_gpo.c
//snip
+int ad_gpo_process_cse_recv(struct tevent_req *req,
TALLOC_CTX *mem_ctx,
int *_allowed_size,
char ***_allowed_sids,
int *_denied_size,
char ***_denied_sids)
+{
- char *smb_uri;
^^^^^^^^^^^^^^ unused variable
- int ret;
- char **allowed_sids;
- int allowed_size;
- char **denied_sids;
- int denied_size;
- struct ad_gpo_process_cse_state *state;
- state = tevent_req_data(req, struct ad_gpo_process_cse_state);
- TEVENT_REQ_RETURN_ON_ERROR(req);
- ret = parse_gpo_child_response(mem_ctx, state->buf, state->len,
&allowed_sids,
&allowed_size,
&denied_sids,
&denied_size);
- if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE,
"Cannot parse child response: [%d][%s]\n", ret,
strerror(ret));
return ret;
- }
- *_allowed_size = allowed_size;
- *_allowed_sids = talloc_steal(mem_ctx, allowed_sids);
- *_denied_size = denied_size;
- *_denied_sids = talloc_steal(mem_ctx, denied_sids);
- return EOK;
+}
+static errno_t +gpo_fork_child(struct tevent_req *req) +{
- int pipefd_to_child[2];
- int pipefd_from_child[2];
- pid_t pid;
- int ret;
- errno_t err;
- struct ad_gpo_process_cse_state *state;
- state = tevent_req_data(req, struct ad_gpo_process_cse_state);
- ret = pipe(pipefd_from_child);
- if (ret == -1) {
err = errno;
DEBUG(SSSDBG_CRIT_FAILURE,
"pipe failed [%d][%s].\n", errno, strerror(errno));
return err;
- }
- ret = pipe(pipefd_to_child);
- if (ret == -1) {
err = errno;
DEBUG(SSSDBG_CRIT_FAILURE,
"pipe failed [%d][%s].\n", errno, strerror(errno));
return err;
- }
- pid = fork();
- if (pid == 0) { /* child */
err = exec_child(state,
pipefd_to_child, pipefd_from_child,
GPO_CHILD, gpo_child_debug_fd);
DEBUG(SSSDBG_CRIT_FAILURE, "Could not exec gpo_child: [%d][%s].\n",
err, strerror(err));
return err;
- } else if (pid > 0) { /* parent */
state->child_pid = pid;
state->io->read_from_child_fd = pipefd_from_child[0];
close(pipefd_from_child[1]);
state->io->write_to_child_fd = pipefd_to_child[1];
close(pipefd_to_child[0]);
fd_nonblocking(state->io->read_from_child_fd);
fd_nonblocking(state->io->write_to_child_fd);
ret = child_handler_setup(state->ev, pid, NULL, NULL, NULL);
if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE,
"Could not set up child signal handler\n");
return ret;
}
- } else { /* error */
err = errno;
DEBUG(SSSDBG_CRIT_FAILURE,
"fork failed [%d][%s].\n", errno, strerror(errno));
return err;
- }
- return EOK;
+} diff --git a/src/providers/ad/ad_gpo_child.c b/src/providers/ad/ad_gpo_child.c new file mode 100644 index 0000000000000000000000000000000000000000..4659d8c3666841ea7bd84194cc1fdcfb671851ed --- /dev/null +++ b/src/providers/ad/ad_gpo_child.c @@ -0,0 +1,620 @@ +/*
- SSSD
- AD GPO Backend Module -- perform SMB and CSE processing in a child
process
- Authors:
Yassir Elley <yelley@redhat.com>
- Copyright (C) 2013 Red Hat
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
- the Free Software Foundation; either version 3 of the License, or
- (at your option) any later version.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program. If not, see http://www.gnu.org/licenses/.
+*/
//snip
+static errno_t +process_security_settings_cse(TALLOC_CTX *mem_ctx,
const char *smb_uri,
char ***_allowed_sids,
int *_allowed_size,
char ***_denied_sids,
int *_denied_size)
+{
- SMBCCTX *context;
- int ret = 0;
- uint8_t *buf = NULL;
- int bytesread = 0;
- char **allowed_sids;
- char **denied_sids;
- int allowed_size = 0;
- int denied_size = 0;
- TALLOC_CTX *tmp_ctx = NULL;
- tmp_ctx = talloc_new(NULL);
- if (tmp_ctx == NULL) {
ret = ENOMEM;
goto done;
You should immediatelly return ENOMEM, otherwise context will be used unitialized in done section.
- }
- DEBUG(SSSDBG_TRACE_ALL, "%s\n", smb_uri);
- context = smbc_new_context();
- if (context == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "Could not allocate new smbc
context\n");
ret = ENOMEM;
goto done;
- }
- smbc_setFunctionAuthData(context, sssd_krb_get_auth_data_fn);
- smbc_setOptionUseKerberos(context, 1);
- /* Initialize the context using the previously specified options */
- if (smbc_init_context(context) == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "Could not initialize smbc context\n");
ret = ENOMEM;
goto done;
- }
- /* Tell the compatibility layer to use this context */
- smbc_set_context(context);
- int remotehandle = smbc_open(smb_uri, O_RDONLY, 0755);
- if (remotehandle < 0) {
DEBUG(SSSDBG_CRIT_FAILURE, "smbc_open failed\n");
ret = EPIPE;
goto done;
- }
- buf = talloc_array(tmp_ctx, uint8_t, SMB_BUFFER_SIZE);
- bytesread = smbc_read(remotehandle, buf, SMB_BUFFER_SIZE);
- if(bytesread < 0) {
DEBUG(SSSDBG_CRIT_FAILURE, "smbc_read failed\n");
ret = EPIPE;
goto done;
- }
- DEBUG(SSSDBG_CRIT_FAILURE, "bytesread: %d\n", bytesread);
- smbc_close(remotehandle);
- ret = ad_gpo_parse_security_cse_buffer(tmp_ctx,
buf,
bytesread,
&allowed_sids,
&allowed_size,
&denied_sids,
&denied_size);
Function ad_gpo_parse_security_cse_buffer can fail in many cases ENOMEM, problem with initialising ini_config_* and output variable will be uninitialized in next lines.
- /* TBD: allowed/denied_sids/size should be stored in cache */
- *_allowed_sids = talloc_steal(mem_ctx, allowed_sids);
- *_allowed_size = allowed_size;
- *_denied_sids = talloc_steal(mem_ctx, denied_sids);
- *_denied_size = denied_size;
- done:
- smbc_free_context(context, 0);
- talloc_free(tmp_ctx);
- return ret;
+}
+int +main(int argc, const char *argv[]) +{
- int opt;
- poptContext pc;
- int debug_fd = -1;
- errno_t ret;
- int result;
- TALLOC_CTX *main_ctx = NULL;
- uint8_t *buf = NULL;
- ssize_t len = 0;
- struct input_buffer *ibuf = NULL;
- struct response *resp = NULL;
- size_t written;
- char **allowed_sids;
- int allowed_size;
- char **denied_sids;
- int denied_size;
- int j;
- struct poptOption long_options[] = {
POPT_AUTOHELP
{"debug-level", 'd', POPT_ARG_INT, &debug_level, 0,
_("Debug level"), NULL},
{"debug-timestamps", 0, POPT_ARG_INT, &debug_timestamps, 0,
_("Add debug timestamps"), NULL},
{"debug-microseconds", 0, POPT_ARG_INT, &debug_microseconds, 0,
_("Show timestamps with microseconds"), NULL},
{"debug-fd", 0, POPT_ARG_INT, &debug_fd, 0,
_("An open file descriptor for the debug logs"), NULL},
POPT_TABLEEND
- };
- /* Set debug level to invalid value so we can decide if -d 0 was used.
*/
- debug_level = SSSDBG_INVALID;
- pc = poptGetContext(argv[0], argc, argv, long_options, 0);
- while((opt = poptGetNextOpt(pc)) != -1) {
switch(opt) {
default:
fprintf(stderr, "\nInvalid option %s: %s\n\n",
poptBadOption(pc, 0), poptStrerror(opt));
poptPrintUsage(pc, stderr, 0);
_exit(-1);
}
- }
- poptFreeContext(pc);
- DEBUG_INIT(debug_level);
- debug_prg_name = talloc_asprintf(NULL, "[sssd[gpo_child[%d]]]",
getpid());
- if (debug_prg_name == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "talloc_asprintf failed.\n");
ret = ENOMEM;
ret is not used in fail. You can ammend previous debug message if you want.
goto fail;
- }
- if (debug_fd != -1) {
ret = set_debug_file_from_fd(debug_fd);
if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE, "set_debug_file_from_fd failed.\n");
}
- }
- DEBUG(SSSDBG_TRACE_FUNC, "gpo_child started.\n");
- main_ctx = talloc_new(NULL);
- if (main_ctx == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new failed.\n");
talloc_free(discard_const(debug_prg_name));
goto fail;
- }
- talloc_steal(main_ctx, debug_prg_name);
- buf = talloc_size(main_ctx, sizeof(uint8_t)*IN_BUF_SIZE);
- if (buf == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "talloc_size failed.\n");
goto fail;
- }
- ibuf = talloc_zero(main_ctx, struct input_buffer);
- if (ibuf == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "talloc_zero failed.\n");
goto fail;
- }
- DEBUG(SSSDBG_TRACE_FUNC, "context initialized\n");
- errno = 0;
- len = sss_atomic_read_s(STDIN_FILENO, buf, IN_BUF_SIZE);
- if (len == -1) {
ret = errno;
DEBUG(SSSDBG_CRIT_FAILURE, "read failed [%d][%s].\n", ret,
strerror(ret));
goto fail;
- }
- close(STDIN_FILENO);
- ret = unpack_buffer(buf, len, ibuf);
- if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE,
"unpack_buffer failed.[%d][%s].\n", ret, strerror(ret));
goto fail;
- }
- DEBUG(SSSDBG_TRACE_FUNC, "processing security settings\n");
- result = process_security_settings_cse(main_ctx,
ibuf->smb_uri,
&allowed_sids,
&allowed_size,
&denied_sids,
&denied_size);
You should test result of function process_security_settings_cse. It may happen that output variables will not be initialised.
- DEBUG(SSSDBG_CRIT_FAILURE, "allowed_size = %d\n", allowed_size);
- for (j= 0; j < allowed_size; j++) {
DEBUG(SSSDBG_CRIT_FAILURE, "allowed_sids[%d] = %s\n", j,
allowed_sids[j]);
- }
- DEBUG(SSSDBG_CRIT_FAILURE, "denied_size = %d\n", denied_size);
- for (j= 0; j < denied_size; j++) {
DEBUG(SSSDBG_CRIT_FAILURE, " denied_sids[%d] = %s\n", j,
denied_sids[j]);
- }
- result = EOK;
Is there a reason for this assignment? Will it be solved with my previous comment?
- ret = prepare_response(main_ctx, result, allowed_size, allowed_sids,
denied_size, denied_sids, &resp);
- if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE, "prepare_response failed. [%d][%s].\n",
ret, strerror(ret));
goto fail;
- }
- errno = 0;
- DEBUG(SSSDBG_TRACE_FUNC, "resp->size: %zu\n", resp->size);
- written = sss_atomic_write_s(STDOUT_FILENO, resp->buf, resp->size);
- if (written == -1) {
ret = errno;
DEBUG(SSSDBG_CRIT_FAILURE, "write failed [%d][%s].\n", ret,
strerror(ret));
goto fail;
- }
- if (written != resp->size) {
DEBUG(SSSDBG_CRIT_FAILURE, "Expected to write %zu bytes, wrote
%zu\n",
resp->size, written);
goto fail;
- }
- DEBUG(SSSDBG_TRACE_FUNC, "gpo_child completed successfully\n");
- close(STDOUT_FILENO);
- talloc_free(main_ctx);
- _exit(0);
It will be better to use constant EXIT_SUCCESS
+fail:
- DEBUG(SSSDBG_CRIT_FAILURE, "gpo_child failed!\n");
- close(STDOUT_FILENO);
- talloc_free(main_ctx);
- _exit(-1);
and here EXIT_FAILURE
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
I have attached a revised patch that addresses Lukas's comments. Only outstanding issue is regarding how to get the name of the DC that SSSD is currently connected to.
Thanks, Yassir.
On Sun, Jun 15, 2014 at 07:08:55PM -0400, Yassir Elley wrote:
- You suggested using the name of the DC that SSSD is currently connected to in the smb uri (rather than the domain.name, which will require libsmbclient to perform a DNS resolution). Is there an easy way to get the name of the DC that SSSD is currently connected to? I am having trouble finding it.
In struct ad_gpo_access_state you have a member struct sdap_id_conn_ctx *conn. conn->service->uri is the LDAP uri for the current connection. You can use calls from OpenLDAP or ldb to split it into components, picj the hostname and create the smb uri.
In general the uri should always be available since you read the GPO data from LDAP before doing the smb operations. Nevertheless you can call be_resolve_server_send() to make sure it is set, see e.g. auth_get_server() how to use it.
HTH
bye, Sumit
----- Original Message -----
On Sun, Jun 15, 2014 at 07:08:55PM -0400, Yassir Elley wrote:
- You suggested using the name of the DC that SSSD is currently connected
to in the smb uri (rather than the domain.name, which will require libsmbclient to perform a DNS resolution). Is there an easy way to get the name of the DC that SSSD is currently connected to? I am having trouble finding it.
In struct ad_gpo_access_state you have a member struct sdap_id_conn_ctx *conn. conn->service->uri is the LDAP uri for the current connection. You can use calls from OpenLDAP or ldb to split it into components, picj the hostname and create the smb uri.
In general the uri should always be available since you read the GPO data from LDAP before doing the smb operations. Nevertheless you can call be_resolve_server_send() to make sure it is set, see e.g. auth_get_server() how to use it.
HTH
bye, Sumit _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
I have attached a revised patch that modifies the smb uri to use the server name rather than the domain name.
Thanks, Yassir.
----- Original Message -----
----- Original Message -----
On Sun, Jun 15, 2014 at 07:08:55PM -0400, Yassir Elley wrote:
- You suggested using the name of the DC that SSSD is currently connected
to in the smb uri (rather than the domain.name, which will require libsmbclient to perform a DNS resolution). Is there an easy way to get the name of the DC that SSSD is currently connected to? I am having trouble finding it.
In struct ad_gpo_access_state you have a member struct sdap_id_conn_ctx *conn. conn->service->uri is the LDAP uri for the current connection. You can use calls from OpenLDAP or ldb to split it into components, picj the hostname and create the smb uri.
In general the uri should always be available since you read the GPO data from LDAP before doing the smb operations. Nevertheless you can call be_resolve_server_send() to make sure it is set, see e.g. auth_get_server() how to use it.
HTH
bye, Sumit _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
I have attached a revised patch that modifies the smb uri to use the server name rather than the domain name.
Thanks, Yassir. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Oops. Forgot to attach the patch.
Yassir.
On Tue, Jun 17, 2014 at 04:44:20PM -0400, Yassir Elley wrote:
----- Original Message -----
----- Original Message -----
On Sun, Jun 15, 2014 at 07:08:55PM -0400, Yassir Elley wrote:
- You suggested using the name of the DC that SSSD is currently connected
to in the smb uri (rather than the domain.name, which will require libsmbclient to perform a DNS resolution). Is there an easy way to get the name of the DC that SSSD is currently connected to? I am having trouble finding it.
In struct ad_gpo_access_state you have a member struct sdap_id_conn_ctx *conn. conn->service->uri is the LDAP uri for the current connection. You can use calls from OpenLDAP or ldb to split it into components, picj the hostname and create the smb uri.
In general the uri should always be available since you read the GPO data from LDAP before doing the smb operations. Nevertheless you can call be_resolve_server_send() to make sure it is set, see e.g. auth_get_server() how to use it.
HTH
bye, Sumit _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
I have attached a revised patch that modifies the smb uri to use the server name rather than the domain name.
Thanks, Yassir. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Oops. Forgot to attach the patch.
Yassir.
Thank you, the patch is working as expected and now uses the hostname to connect to the DC. But please use e.g. ldap_url_parse() from OpenLDAP to split the url and take the hostname from the lud_host member of typedef struct ldap_url_desc. The LDAP url can contain port numbers which would currently cause troubles with your scheme.
As a general comment, please try to split your patches into smaller units. This would help to review them especially to compare multiple versions of a patch.
I have not looked at the child code in details yet, but I would like to suggest a change in the workflow. I think the child should only download the gpo file and save it at some place, e.g. /var/lib/sss/gpo_cache/ and then the backend will read an process it. This way you already have the file available in the offline case. When calling the child the backend should provide the smb url and a location to store the result. The child can e.g. return a checksum for the file which the backend can save together with the download time in the sysdb cache in a subtree below cn=custom (grep sysdb.h for 'custom' to find the related sysdb calls). With the download time it would be even possible to specific cache lifetime during which the gpo file will not be downloaded again to save bandwidth. But this should be optional.
What do you think?
bye, Sumit
On Wed, 2014-06-18 at 14:50 +0200, Sumit Bose wrote:
On Tue, Jun 17, 2014 at 04:44:20PM -0400, Yassir Elley wrote:
----- Original Message -----
----- Original Message -----
On Sun, Jun 15, 2014 at 07:08:55PM -0400, Yassir Elley wrote:
- You suggested using the name of the DC that SSSD is currently connected
to in the smb uri (rather than the domain.name, which will require libsmbclient to perform a DNS resolution). Is there an easy way to get the name of the DC that SSSD is currently connected to? I am having trouble finding it.
In struct ad_gpo_access_state you have a member struct sdap_id_conn_ctx *conn. conn->service->uri is the LDAP uri for the current connection. You can use calls from OpenLDAP or ldb to split it into components, picj the hostname and create the smb uri.
In general the uri should always be available since you read the GPO data from LDAP before doing the smb operations. Nevertheless you can call be_resolve_server_send() to make sure it is set, see e.g. auth_get_server() how to use it.
HTH
bye, Sumit _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
I have attached a revised patch that modifies the smb uri to use the server name rather than the domain name.
Thanks, Yassir. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Oops. Forgot to attach the patch.
Yassir.
Thank you, the patch is working as expected and now uses the hostname to connect to the DC. But please use e.g. ldap_url_parse() from OpenLDAP to split the url and take the hostname from the lud_host member of typedef struct ldap_url_desc. The LDAP url can contain port numbers which would currently cause troubles with your scheme.
As a general comment, please try to split your patches into smaller units. This would help to review them especially to compare multiple versions of a patch.
I have not looked at the child code in details yet, but I would like to suggest a change in the workflow. I think the child should only download the gpo file and save it at some place, e.g. /var/lib/sss/gpo_cache/ and then the backend will read an process it. This way you already have the file available in the offline case. When calling the child the backend should provide the smb url and a location to store the result. The child can e.g. return a checksum for the file which the backend can save together with the download time in the sysdb cache in a subtree below cn=custom (grep sysdb.h for 'custom' to find the related sysdb calls). With the download time it would be even possible to specific cache lifetime during which the gpo file will not be downloaded again to save bandwidth. But this should be optional.
What do you think?
Looks like excellent advice all around to me.
Simo.
----- Original Message -----
On Tue, Jun 17, 2014 at 04:44:20PM -0400, Yassir Elley wrote:
----- Original Message -----
----- Original Message -----
On Sun, Jun 15, 2014 at 07:08:55PM -0400, Yassir Elley wrote:
- You suggested using the name of the DC that SSSD is currently
connected to in the smb uri (rather than the domain.name, which will require libsmbclient to perform a DNS resolution). Is there an easy way to get the name of the DC that SSSD is currently connected to? I am having trouble finding it.
In struct ad_gpo_access_state you have a member struct sdap_id_conn_ctx *conn. conn->service->uri is the LDAP uri for the current connection. You can use calls from OpenLDAP or ldb to split it into components, picj the hostname and create the smb uri.
In general the uri should always be available since you read the GPO data from LDAP before doing the smb operations. Nevertheless you can call be_resolve_server_send() to make sure it is set, see e.g. auth_get_server() how to use it.
HTH
bye, Sumit _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
I have attached a revised patch that modifies the smb uri to use the server name rather than the domain name.
Thanks, Yassir. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Oops. Forgot to attach the patch.
Yassir.
Thank you, the patch is working as expected and now uses the hostname to connect to the DC. But please use e.g. ldap_url_parse() from OpenLDAP to split the url and take the hostname from the lud_host member of typedef struct ldap_url_desc. The LDAP url can contain port numbers which would currently cause troubles with your scheme.
As a general comment, please try to split your patches into smaller units. This would help to review them especially to compare multiple versions of a patch.
I thought you had previously indicated a preference for a single patch (with the understanding that this makes it more difficult for the reviewer). Perhaps I misunderstood. In any case, I have attached two patches to this email: the previous patch, and a new patch to address your comment about using ldap_url_parse().
I have not looked at the child code in details yet, but I would like to suggest a change in the workflow. I think the child should only download the gpo file and save it at some place, e.g. /var/lib/sss/gpo_cache/ and then the backend will read an process it. This way you already have the file available in the offline case. When calling the child the backend should provide the smb url and a location to store the result. The child can e.g. return a checksum for the file which the backend can save together with the download time in the sysdb cache in a subtree below cn=custom (grep sysdb.h for 'custom' to find the related sysdb calls). With the download time it would be even possible to specific cache lifetime during which the gpo file will not be downloaded again to save bandwidth. But this should be optional.
What do you think?
bye, Sumit _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
With regard to your suggestion to store the policy data in a local file (and store the file's metadata in the cache), I think it would be preferrable to have the child parse the file, and store the relevant data/metadata directly to the cache. I believe this is how IPA HBAC currently works (i.e. storing to cache, rather than to file). This would allow the file parsing to be done once (when it is written by the child), rather than the file parsing being done every time the policy is processed (when it is read by the backend). Additionally, keep in mind that we are interested in only a subset of the stanzas (i.e. "Privilege Rights" stanza) in the Security Settings policy file (GptTmpl.inf). In other words, only a small portion of the file contains data that is relevant for us.
For this initial patch, I am having the child return the allowed_sids and denied_sids to the backend for further processing. However, in subsequent patches, I am planning on having the child store the relevant policy data to the cache, and then have the backend retrieve the relevant policy data from the cache. In other words, the proposed logic for future patches is: * if we are online, the backend calls the child (which stores the parsed data to cache) * regardless of whether we are online or offline, the backend retrieves the parsed data from the cache and makes a policy decision.
With regard to reducing unnecessary downloads, I am planning on implementing the logic suggested by [MS-GPOL] of comparing the cached gpo version against the current gpo version (by downloading only the GPT.INI file). If the gpo versions are the same, there is no need to download the policy file. Again, this is intended for a subsequent patch.
Regards, Yassir.
On Wed, Jun 18, 2014 at 01:50:17PM -0400, Yassir Elley wrote:
----- Original Message -----
On Tue, Jun 17, 2014 at 04:44:20PM -0400, Yassir Elley wrote:
----- Original Message -----
----- Original Message -----
On Sun, Jun 15, 2014 at 07:08:55PM -0400, Yassir Elley wrote:
- You suggested using the name of the DC that SSSD is currently
connected to in the smb uri (rather than the domain.name, which will require libsmbclient to perform a DNS resolution). Is there an easy way to get the name of the DC that SSSD is currently connected to? I am having trouble finding it.
In struct ad_gpo_access_state you have a member struct sdap_id_conn_ctx *conn. conn->service->uri is the LDAP uri for the current connection. You can use calls from OpenLDAP or ldb to split it into components, picj the hostname and create the smb uri.
In general the uri should always be available since you read the GPO data from LDAP before doing the smb operations. Nevertheless you can call be_resolve_server_send() to make sure it is set, see e.g. auth_get_server() how to use it.
HTH
bye, Sumit _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
I have attached a revised patch that modifies the smb uri to use the server name rather than the domain name.
Thanks, Yassir. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Oops. Forgot to attach the patch.
Yassir.
Thank you, the patch is working as expected and now uses the hostname to connect to the DC. But please use e.g. ldap_url_parse() from OpenLDAP to split the url and take the hostname from the lud_host member of typedef struct ldap_url_desc. The LDAP url can contain port numbers which would currently cause troubles with your scheme.
As a general comment, please try to split your patches into smaller units. This would help to review them especially to compare multiple versions of a patch.
I thought you had previously indicated a preference for a single patch (with the understanding that this makes it more difficult for the reviewer). Perhaps I misunderstood. In any case, I have attached two patches to this email: the previous patch, and a new patch to address your comment about using ldap_url_parse().
I have not looked at the child code in details yet, but I would like to suggest a change in the workflow. I think the child should only download the gpo file and save it at some place, e.g. /var/lib/sss/gpo_cache/ and then the backend will read an process it. This way you already have the file available in the offline case. When calling the child the backend should provide the smb url and a location to store the result. The child can e.g. return a checksum for the file which the backend can save together with the download time in the sysdb cache in a subtree below cn=custom (grep sysdb.h for 'custom' to find the related sysdb calls). With the download time it would be even possible to specific cache lifetime during which the gpo file will not be downloaded again to save bandwidth. But this should be optional.
What do you think?
bye, Sumit _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
With regard to your suggestion to store the policy data in a local file (and store the file's metadata in the cache), I think it would be preferrable to have the child parse the file, and store the relevant data/metadata directly to the cache. I believe this is how IPA HBAC currently works (i.e. storing to cache, rather than to file). This
yes, but the original IPA HBAC data comes from a LDAP tree so storing it to the cache which has LDAP semantics as well is natural.
would allow the file parsing to be done once (when it is written by the child), rather than the file parsing being done every time the policy is processed (when it is read by the backend). Additionally,
That's a good point. Maybe be can do both, the child just reads and saves the file and the backend parses it once and save the access control data to the cache so that it has to parse the file again only when a new version is read by the child.
keep in mind that we are interested in only a subset of the stanzas (i.e. "Privilege Rights" stanza) in the Security Settings policy file (GptTmpl.inf). In other words, only a small portion of the file contains data that is relevant for us.
yes, nevertheless I'd like to keep the file around. E.g. it would help to make debugging easier because we do not have to tell users how to download the file from the server to check to content. Additionally other application might be interested in the content as well (they can even ensure that the version of the file is recent by calling pam_acct_mgmt() which would trigger the access provider in the SSSD backend, I agree that this is not the nicest solution :-).
For this initial patch, I am having the child return the allowed_sids and denied_sids to the backend for further processing. However, in subsequent patches, I am planning on having the child store the relevant policy data to the cache, and then have the backend retrieve the relevant policy data from the cache. In other words, the proposed logic for future patches is:
- if we are online, the backend calls the child (which stores the parsed data to cache)
- regardless of whether we are online or offline, the backend retrieves the parsed data from the cache and makes a policy decision.
Please do not read/write form/to the cache from the child. So far we tried to limit the child processes to only do what is blocking and cannot be one in an asynchronous ways. Currently this are mostly Kerberos related operations like getting TGTs or updating DNS with GSS-TSIG.
With regard to reducing unnecessary downloads, I am planning on implementing the logic suggested by [MS-GPOL] of comparing the cached gpo version against the current gpo version (by downloading only the GPT.INI file). If the gpo versions are the same, there is no need to download the policy file. Again, this is intended for a subsequent patch.
I think the term 'bandwidth' was not chosen well in my comment. I'm not much concerned by the actual download itself but the general costs including creating the connection, doing Kerberos authentication etc. Please note that Windows client might stay connected to the sysvol and might have some clever mechanism to make it cheap to check GPT.INI every time. I think from our point of view just reading GPT.INI or reading GptTmpl.inf shouldn't make much difference in the general case. So I still would suggest to introduce an SSSD specific timeout. But I won't mind checking GPT.INI as well. So the version of the policy should be written to the cache as well and send to the child so that it first can download GPT.INI and check if the version changed. If the version has not changed it should indicate this to the backend so that it know it does not has to read and parse the policy file (if we agree that the backend should do this :-).
bye, Sumit
Regards, Yassir.
----- Original Message -----
On Wed, Jun 18, 2014 at 01:50:17PM -0400, Yassir Elley wrote:
----- Original Message -----
On Tue, Jun 17, 2014 at 04:44:20PM -0400, Yassir Elley wrote:
----- Original Message -----
----- Original Message -----
On Sun, Jun 15, 2014 at 07:08:55PM -0400, Yassir Elley wrote: > > > * You suggested using the name of the DC that SSSD is currently > connected > to in the smb uri (rather than the domain.name, which will > require > libsmbclient to perform a DNS resolution). Is there an easy way > to > get > the > name of the DC that SSSD is currently connected to? I am having > trouble > finding it. >
In struct ad_gpo_access_state you have a member struct sdap_id_conn_ctx *conn. conn->service->uri is the LDAP uri for the current connection. You can use calls from OpenLDAP or ldb to split it into components, picj the hostname and create the smb uri.
In general the uri should always be available since you read the GPO data from LDAP before doing the smb operations. Nevertheless you can call be_resolve_server_send() to make sure it is set, see e.g. auth_get_server() how to use it.
HTH
bye, Sumit _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
I have attached a revised patch that modifies the smb uri to use the server name rather than the domain name.
Thanks, Yassir. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Oops. Forgot to attach the patch.
Yassir.
Thank you, the patch is working as expected and now uses the hostname to connect to the DC. But please use e.g. ldap_url_parse() from OpenLDAP to split the url and take the hostname from the lud_host member of typedef struct ldap_url_desc. The LDAP url can contain port numbers which would currently cause troubles with your scheme.
As a general comment, please try to split your patches into smaller units. This would help to review them especially to compare multiple versions of a patch.
I thought you had previously indicated a preference for a single patch (with the understanding that this makes it more difficult for the reviewer). Perhaps I misunderstood. In any case, I have attached two patches to this email: the previous patch, and a new patch to address your comment about using ldap_url_parse().
I have not looked at the child code in details yet, but I would like to suggest a change in the workflow. I think the child should only download the gpo file and save it at some place, e.g. /var/lib/sss/gpo_cache/ and then the backend will read an process it. This way you already have the file available in the offline case. When calling the child the backend should provide the smb url and a location to store the result. The child can e.g. return a checksum for the file which the backend can save together with the download time in the sysdb cache in a subtree below cn=custom (grep sysdb.h for 'custom' to find the related sysdb calls). With the download time it would be even possible to specific cache lifetime during which the gpo file will not be downloaded again to save bandwidth. But this should be optional.
What do you think?
bye, Sumit _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
With regard to your suggestion to store the policy data in a local file (and store the file's metadata in the cache), I think it would be preferrable to have the child parse the file, and store the relevant data/metadata directly to the cache. I believe this is how IPA HBAC currently works (i.e. storing to cache, rather than to file). This
yes, but the original IPA HBAC data comes from a LDAP tree so storing it to the cache which has LDAP semantics as well is natural.
would allow the file parsing to be done once (when it is written by the child), rather than the file parsing being done every time the policy is processed (when it is read by the backend). Additionally,
That's a good point. Maybe be can do both, the child just reads and saves the file and the backend parses it once and save the access control data to the cache so that it has to parse the file again only when a new version is read by the child.
keep in mind that we are interested in only a subset of the stanzas (i.e. "Privilege Rights" stanza) in the Security Settings policy file (GptTmpl.inf). In other words, only a small portion of the file contains data that is relevant for us.
yes, nevertheless I'd like to keep the file around. E.g. it would help to make debugging easier because we do not have to tell users how to download the file from the server to check to content. Additionally other application might be interested in the content as well (they can even ensure that the version of the file is recent by calling pam_acct_mgmt() which would trigger the access provider in the SSSD backend, I agree that this is not the nicest solution :-).
For this initial patch, I am having the child return the allowed_sids and denied_sids to the backend for further processing. However, in subsequent patches, I am planning on having the child store the relevant policy data to the cache, and then have the backend retrieve the relevant policy data from the cache. In other words, the proposed logic for future patches is:
- if we are online, the backend calls the child (which stores the parsed
data to cache)
- regardless of whether we are online or offline, the backend retrieves the
parsed data from the cache and makes a policy decision.
Please do not read/write form/to the cache from the child. So far we tried to limit the child processes to only do what is blocking and cannot be one in an asynchronous ways. Currently this are mostly Kerberos related operations like getting TGTs or updating DNS with GSS-TSIG.
You cite two reasons to save the policy data as a local file: 1) to have the local file available as an artifact for debugging and/or consumption by other applications. 2) to keep processing in the child to the bare minimum (i.e. the child shouldn't cache the policy data)
The first reason doesn't seem very compelling IMO. It is unclear to me why being able to consult the policy file (which includes lots of extraneous policy data) would be better than simply relying on the debug logs (which contain only the relevant policy data). Also, I am not aware of any applications (other than the sssd-ad provider) that are planning on using this policy data. Do you have particular application(s) in mind? The second reason is more compelling. However, if we have the child retrieve the data, parse the data, and return the parsed data to the backend, then the child would not need to interact with the cache (i.e. only the backend would interact with the cache).
The reason I am pushing back on the suggestion to store the data to a file is that it adds complexity without clear benefits IMO (unless I am missing something). For example, if we agree that the backend should parse the data and store it to cache anyway (at least when there is a new version), why not save the step of writing to a local file, by having the child parse the data, and having the backend store the data to cache. Additionally, exposing the *entire* contents of the policy file for debugging purposes, when we are only using a few fields, seems like it would only confuse users.
With regard to reducing unnecessary downloads, I am planning on implementing the logic suggested by [MS-GPOL] of comparing the cached gpo version against the current gpo version (by downloading only the GPT.INI file). If the gpo versions are the same, there is no need to download the policy file. Again, this is intended for a subsequent patch.
I think the term 'bandwidth' was not chosen well in my comment. I'm not much concerned by the actual download itself but the general costs including creating the connection, doing Kerberos authentication etc. Please note that Windows client might stay connected to the sysvol and might have some clever mechanism to make it cheap to check GPT.INI every time. I think from our point of view just reading GPT.INI or reading GptTmpl.inf shouldn't make much difference in the general case. So I still would suggest to introduce an SSSD specific timeout. But I won't mind checking GPT.INI as well. So the version of the policy should be written to the cache as well and send to the child so that it first can download GPT.INI and check if the version changed. If the version has not changed it should indicate this to the backend so that it know it does not has to read and parse the policy file (if we agree that the backend should do this :-).
In addition to the GPT.INI check, I agree that adding an SSSD specific timeout could be useful. Additionally, the backend should probably send *all* the guid-filtered GPOs to the child at once (so that the child can create a single smb connection, and re-use it for retrieving all the relevant GPT.INI and GptTmpl.inf files). What do you think?
Regards, Yassir.
On Thu, Jun 19, 2014 at 03:06:04PM -0400, Yassir Elley wrote:
----- Original Message -----
On Wed, Jun 18, 2014 at 01:50:17PM -0400, Yassir Elley wrote:
----- Original Message -----
On Tue, Jun 17, 2014 at 04:44:20PM -0400, Yassir Elley wrote:
----- Original Message -----
----- Original Message ----- > On Sun, Jun 15, 2014 at 07:08:55PM -0400, Yassir Elley wrote: > > > > > > * You suggested using the name of the DC that SSSD is currently > > connected > > to in the smb uri (rather than the domain.name, which will > > require > > libsmbclient to perform a DNS resolution). Is there an easy way > > to > > get > > the > > name of the DC that SSSD is currently connected to? I am having > > trouble > > finding it. > > > > In struct ad_gpo_access_state you have a member struct > sdap_id_conn_ctx > *conn. conn->service->uri is the LDAP uri for the current > connection. > You can use calls from OpenLDAP or ldb to split it into components, > picj > the hostname and create the smb uri. > > In general the uri should always be available since you read the > GPO > data from LDAP before doing the smb operations. Nevertheless you > can > call be_resolve_server_send() to make sure it is set, see e.g. > auth_get_server() how to use it. > > HTH > > bye, > Sumit > _______________________________________________ > sssd-devel mailing list > sssd-devel@lists.fedorahosted.org > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel >
I have attached a revised patch that modifies the smb uri to use the server name rather than the domain name.
Thanks, Yassir. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Oops. Forgot to attach the patch.
Yassir.
Thank you, the patch is working as expected and now uses the hostname to connect to the DC. But please use e.g. ldap_url_parse() from OpenLDAP to split the url and take the hostname from the lud_host member of typedef struct ldap_url_desc. The LDAP url can contain port numbers which would currently cause troubles with your scheme.
As a general comment, please try to split your patches into smaller units. This would help to review them especially to compare multiple versions of a patch.
I thought you had previously indicated a preference for a single patch (with the understanding that this makes it more difficult for the reviewer). Perhaps I misunderstood. In any case, I have attached two patches to this email: the previous patch, and a new patch to address your comment about using ldap_url_parse().
I have not looked at the child code in details yet, but I would like to suggest a change in the workflow. I think the child should only download the gpo file and save it at some place, e.g. /var/lib/sss/gpo_cache/ and then the backend will read an process it. This way you already have the file available in the offline case. When calling the child the backend should provide the smb url and a location to store the result. The child can e.g. return a checksum for the file which the backend can save together with the download time in the sysdb cache in a subtree below cn=custom (grep sysdb.h for 'custom' to find the related sysdb calls). With the download time it would be even possible to specific cache lifetime during which the gpo file will not be downloaded again to save bandwidth. But this should be optional.
What do you think?
bye, Sumit _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
With regard to your suggestion to store the policy data in a local file (and store the file's metadata in the cache), I think it would be preferrable to have the child parse the file, and store the relevant data/metadata directly to the cache. I believe this is how IPA HBAC currently works (i.e. storing to cache, rather than to file). This
yes, but the original IPA HBAC data comes from a LDAP tree so storing it to the cache which has LDAP semantics as well is natural.
would allow the file parsing to be done once (when it is written by the child), rather than the file parsing being done every time the policy is processed (when it is read by the backend). Additionally,
That's a good point. Maybe be can do both, the child just reads and saves the file and the backend parses it once and save the access control data to the cache so that it has to parse the file again only when a new version is read by the child.
keep in mind that we are interested in only a subset of the stanzas (i.e. "Privilege Rights" stanza) in the Security Settings policy file (GptTmpl.inf). In other words, only a small portion of the file contains data that is relevant for us.
yes, nevertheless I'd like to keep the file around. E.g. it would help to make debugging easier because we do not have to tell users how to download the file from the server to check to content. Additionally other application might be interested in the content as well (they can even ensure that the version of the file is recent by calling pam_acct_mgmt() which would trigger the access provider in the SSSD backend, I agree that this is not the nicest solution :-).
For this initial patch, I am having the child return the allowed_sids and denied_sids to the backend for further processing. However, in subsequent patches, I am planning on having the child store the relevant policy data to the cache, and then have the backend retrieve the relevant policy data from the cache. In other words, the proposed logic for future patches is:
- if we are online, the backend calls the child (which stores the parsed
data to cache)
- regardless of whether we are online or offline, the backend retrieves the
parsed data from the cache and makes a policy decision.
Please do not read/write form/to the cache from the child. So far we tried to limit the child processes to only do what is blocking and cannot be one in an asynchronous ways. Currently this are mostly Kerberos related operations like getting TGTs or updating DNS with GSS-TSIG.
You cite two reasons to save the policy data as a local file:
- to have the local file available as an artifact for debugging and/or consumption by other applications.
- to keep processing in the child to the bare minimum (i.e. the child shouldn't cache the policy data)
The first reason doesn't seem very compelling IMO. It is unclear to me why being able to consult the policy file (which includes lots of extraneous policy data) would be better than simply relying on the debug logs (which contain only the relevant policy data). Also, I am not aware of any applications (other than the sssd-ad provider) that are planning on using this policy data. Do you have particular application(s) in mind? The second reason is more compelling. However, if we have the child retrieve the data, parse the data, and return the parsed data to the backend, then the child would not need to interact with the cache (i.e. only the backend would interact with the cache).
The reason I am pushing back on the suggestion to store the data to a file is that it adds complexity without clear benefits IMO (unless I am missing something). For example, if we agree that the backend should parse the data and store it to cache anyway (at least when there is a new version), why not save the step of writing to a local file, by having the child parse the data, and having the backend store the data to cache. Additionally, exposing the *entire* contents of the policy file for debugging purposes, when we are only using a few fields, seems like it would only confuse users.
What about the third one about other applications. Since SSSD is already doing the work of figuring out which GPOs apply to the host and loading the data why not making the data available to others?
bye, Sumit
With regard to reducing unnecessary downloads, I am planning on implementing the logic suggested by [MS-GPOL] of comparing the cached gpo version against the current gpo version (by downloading only the GPT.INI file). If the gpo versions are the same, there is no need to download the policy file. Again, this is intended for a subsequent patch.
I think the term 'bandwidth' was not chosen well in my comment. I'm not much concerned by the actual download itself but the general costs including creating the connection, doing Kerberos authentication etc. Please note that Windows client might stay connected to the sysvol and might have some clever mechanism to make it cheap to check GPT.INI every time. I think from our point of view just reading GPT.INI or reading GptTmpl.inf shouldn't make much difference in the general case. So I still would suggest to introduce an SSSD specific timeout. But I won't mind checking GPT.INI as well. So the version of the policy should be written to the cache as well and send to the child so that it first can download GPT.INI and check if the version changed. If the version has not changed it should indicate this to the backend so that it know it does not has to read and parse the policy file (if we agree that the backend should do this :-).
In addition to the GPT.INI check, I agree that adding an SSSD specific timeout could be useful. Additionally, the backend should probably send *all* the guid-filtered GPOs to the child at once (so that the child can create a single smb connection, and re-use it for retrieving all the relevant GPT.INI and GptTmpl.inf files). What do you think?
Regards, Yassir. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
----- Original Message -----
On Thu, Jun 19, 2014 at 03:06:04PM -0400, Yassir Elley wrote:
----- Original Message -----
On Wed, Jun 18, 2014 at 01:50:17PM -0400, Yassir Elley wrote:
----- Original Message -----
On Tue, Jun 17, 2014 at 04:44:20PM -0400, Yassir Elley wrote:
----- Original Message ----- > > > ----- Original Message ----- > > On Sun, Jun 15, 2014 at 07:08:55PM -0400, Yassir Elley wrote: > > > > > > > > > * You suggested using the name of the DC that SSSD is > > > currently > > > connected > > > to in the smb uri (rather than the domain.name, which will > > > require > > > libsmbclient to perform a DNS resolution). Is there an easy > > > way > > > to > > > get > > > the > > > name of the DC that SSSD is currently connected to? I am > > > having > > > trouble > > > finding it. > > > > > > > In struct ad_gpo_access_state you have a member struct > > sdap_id_conn_ctx > > *conn. conn->service->uri is the LDAP uri for the current > > connection. > > You can use calls from OpenLDAP or ldb to split it into > > components, > > picj > > the hostname and create the smb uri. > > > > In general the uri should always be available since you read > > the > > GPO > > data from LDAP before doing the smb operations. Nevertheless > > you > > can > > call be_resolve_server_send() to make sure it is set, see e.g. > > auth_get_server() how to use it. > > > > HTH > > > > bye, > > Sumit > > _______________________________________________ > > sssd-devel mailing list > > sssd-devel@lists.fedorahosted.org > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel > > > > I have attached a revised patch that modifies the smb uri to use > the > server > name rather than the domain name. > > Thanks, > Yassir. > _______________________________________________ > sssd-devel mailing list > sssd-devel@lists.fedorahosted.org > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel >
Oops. Forgot to attach the patch.
Yassir.
Thank you, the patch is working as expected and now uses the hostname to connect to the DC. But please use e.g. ldap_url_parse() from OpenLDAP to split the url and take the hostname from the lud_host member of typedef struct ldap_url_desc. The LDAP url can contain port numbers which would currently cause troubles with your scheme.
As a general comment, please try to split your patches into smaller units. This would help to review them especially to compare multiple versions of a patch.
I thought you had previously indicated a preference for a single patch (with the understanding that this makes it more difficult for the reviewer). Perhaps I misunderstood. In any case, I have attached two patches to this email: the previous patch, and a new patch to address your comment about using ldap_url_parse().
I have not looked at the child code in details yet, but I would like to suggest a change in the workflow. I think the child should only download the gpo file and save it at some place, e.g. /var/lib/sss/gpo_cache/ and then the backend will read an process it. This way you already have the file available in the offline case. When calling the child the backend should provide the smb url and a location to store the result. The child can e.g. return a checksum for the file which the backend can save together with the download time in the sysdb cache in a subtree below cn=custom (grep sysdb.h for 'custom' to find the related sysdb calls). With the download time it would be even possible to specific cache lifetime during which the gpo file will not be downloaded again to save bandwidth. But this should be optional.
What do you think?
bye, Sumit _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
With regard to your suggestion to store the policy data in a local file (and store the file's metadata in the cache), I think it would be preferrable to have the child parse the file, and store the relevant data/metadata directly to the cache. I believe this is how IPA HBAC currently works (i.e. storing to cache, rather than to file). This
yes, but the original IPA HBAC data comes from a LDAP tree so storing it to the cache which has LDAP semantics as well is natural.
would allow the file parsing to be done once (when it is written by the child), rather than the file parsing being done every time the policy is processed (when it is read by the backend). Additionally,
That's a good point. Maybe be can do both, the child just reads and saves the file and the backend parses it once and save the access control data to the cache so that it has to parse the file again only when a new version is read by the child.
keep in mind that we are interested in only a subset of the stanzas (i.e. "Privilege Rights" stanza) in the Security Settings policy file (GptTmpl.inf). In other words, only a small portion of the file contains data that is relevant for us.
yes, nevertheless I'd like to keep the file around. E.g. it would help to make debugging easier because we do not have to tell users how to download the file from the server to check to content. Additionally other application might be interested in the content as well (they can even ensure that the version of the file is recent by calling pam_acct_mgmt() which would trigger the access provider in the SSSD backend, I agree that this is not the nicest solution :-).
For this initial patch, I am having the child return the allowed_sids and denied_sids to the backend for further processing. However, in subsequent patches, I am planning on having the child store the relevant policy data to the cache, and then have the backend retrieve the relevant policy data from the cache. In other words, the proposed logic for future patches is:
- if we are online, the backend calls the child (which stores the
parsed data to cache)
- regardless of whether we are online or offline, the backend retrieves
the parsed data from the cache and makes a policy decision.
Please do not read/write form/to the cache from the child. So far we tried to limit the child processes to only do what is blocking and cannot be one in an asynchronous ways. Currently this are mostly Kerberos related operations like getting TGTs or updating DNS with GSS-TSIG.
You cite two reasons to save the policy data as a local file:
- to have the local file available as an artifact for debugging and/or
consumption by other applications. 2) to keep processing in the child to the bare minimum (i.e. the child shouldn't cache the policy data)
The first reason doesn't seem very compelling IMO. It is unclear to me why being able to consult the policy file (which includes lots of extraneous policy data) would be better than simply relying on the debug logs (which contain only the relevant policy data). Also, I am not aware of any applications (other than the sssd-ad provider) that are planning on using this policy data. Do you have particular application(s) in mind? The second reason is more compelling. However, if we have the child retrieve the data, parse the data, and return the parsed data to the backend, then the child would not need to interact with the cache (i.e. only the backend would interact with the cache).
The reason I am pushing back on the suggestion to store the data to a file is that it adds complexity without clear benefits IMO (unless I am missing something). For example, if we agree that the backend should parse the data and store it to cache anyway (at least when there is a new version), why not save the step of writing to a local file, by having the child parse the data, and having the backend store the data to cache. Additionally, exposing the *entire* contents of the policy file for debugging purposes, when we are only using a few fields, seems like it would only confuse users.
What about the third one about other applications. Since SSSD is already doing the work of figuring out which GPOs apply to the host and loading the data why not making the data available to others?
OK. I agree that this is compelling enough to use a file-based approach.
With regard to reducing unnecessary downloads, I am planning on implementing the logic suggested by [MS-GPOL] of comparing the cached gpo version against the current gpo version (by downloading only the GPT.INI file). If the gpo versions are the same, there is no need to download the policy file. Again, this is intended for a subsequent patch.
I think the term 'bandwidth' was not chosen well in my comment. I'm not much concerned by the actual download itself but the general costs including creating the connection, doing Kerberos authentication etc. Please note that Windows client might stay connected to the sysvol and might have some clever mechanism to make it cheap to check GPT.INI every time. I think from our point of view just reading GPT.INI or reading GptTmpl.inf shouldn't make much difference in the general case. So I still would suggest to introduce an SSSD specific timeout. But I won't mind checking GPT.INI as well. So the version of the policy should be written to the cache as well and send to the child so that it first can download GPT.INI and check if the version changed. If the version has not changed it should indicate this to the backend so that it know it does not has to read and parse the policy file (if we agree that the backend should do this :-).
In addition to the GPT.INI check, I agree that adding an SSSD specific timeout could be useful. Additionally, the backend should probably send *all* the guid-filtered GPOs to the child at once (so that the child can create a single smb connection, and re-use it for retrieving all the relevant GPT.INI and GptTmpl.inf files). What do you think?
I think my concern about performance could just be based on my current design of the gpo_child. Currently, for *each* smb_uri, I fork/exec a new gpo_child process, send it the smb_uri, have it create an smb connection, perform the smb retrieval, return some data, and then exit the gpo_child process. If gpo-ldap determines that multiple gpos are applicable, the current design could be problematic, as it requires multiple gpo_child processes to be spawned and killed (i.e. one gpo_child process per smb_uri). I am only using this design b/c that is how the ldap_child and krb5_child seem to operate.
Perhaps a better approach would be: to continue to call the gpo_child separately for each smb_uri, but to only launch a single gpo_child process to handle all the gpos related to a single policy application. This would do away with spawning multiple processes; it would also allow the single gpo_child to create and maintain a single SMB connection, at least for the duration of a single policy application. Does this fit in with the design philosophy of minimalist child processes? Or would it be preferrable to just stay with launching a new child process for each smb_uri?
On a separate topic, I am wondering if we should complete the review of the current patch and push it to master, before I potentially add a file-based approached and/or an enhanced gpo_child design. This would keep the current patch from growing even bigger. Also, the current patch produces correct results, so others could play with it (even if we are planning on changing to a file-based approach later on). What do you think?
Regards, Yassir.
On Mon, Jun 23, 2014 at 08:52:53PM -0400, Yassir Elley wrote:
----- Original Message -----
On Thu, Jun 19, 2014 at 03:06:04PM -0400, Yassir Elley wrote:
----- Original Message -----
On Wed, Jun 18, 2014 at 01:50:17PM -0400, Yassir Elley wrote:
----- Original Message -----
On Tue, Jun 17, 2014 at 04:44:20PM -0400, Yassir Elley wrote: > > > ----- Original Message ----- > > > > > > ----- Original Message ----- > > > On Sun, Jun 15, 2014 at 07:08:55PM -0400, Yassir Elley wrote: > > > > > > > > > > > > * You suggested using the name of the DC that SSSD is > > > > currently > > > > connected > > > > to in the smb uri (rather than the domain.name, which will > > > > require > > > > libsmbclient to perform a DNS resolution). Is there an easy > > > > way > > > > to > > > > get > > > > the > > > > name of the DC that SSSD is currently connected to? I am > > > > having > > > > trouble > > > > finding it. > > > > > > > > > > In struct ad_gpo_access_state you have a member struct > > > sdap_id_conn_ctx > > > *conn. conn->service->uri is the LDAP uri for the current > > > connection. > > > You can use calls from OpenLDAP or ldb to split it into > > > components, > > > picj > > > the hostname and create the smb uri. > > > > > > In general the uri should always be available since you read > > > the > > > GPO > > > data from LDAP before doing the smb operations. Nevertheless > > > you > > > can > > > call be_resolve_server_send() to make sure it is set, see e.g. > > > auth_get_server() how to use it. > > > > > > HTH > > > > > > bye, > > > Sumit > > > _______________________________________________ > > > sssd-devel mailing list > > > sssd-devel@lists.fedorahosted.org > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel > > > > > > > I have attached a revised patch that modifies the smb uri to use > > the > > server > > name rather than the domain name. > > > > Thanks, > > Yassir. > > _______________________________________________ > > sssd-devel mailing list > > sssd-devel@lists.fedorahosted.org > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel > > > > Oops. Forgot to attach the patch. > > Yassir.
Thank you, the patch is working as expected and now uses the hostname to connect to the DC. But please use e.g. ldap_url_parse() from OpenLDAP to split the url and take the hostname from the lud_host member of typedef struct ldap_url_desc. The LDAP url can contain port numbers which would currently cause troubles with your scheme.
As a general comment, please try to split your patches into smaller units. This would help to review them especially to compare multiple versions of a patch.
I thought you had previously indicated a preference for a single patch (with the understanding that this makes it more difficult for the reviewer). Perhaps I misunderstood. In any case, I have attached two patches to this email: the previous patch, and a new patch to address your comment about using ldap_url_parse().
I have not looked at the child code in details yet, but I would like to suggest a change in the workflow. I think the child should only download the gpo file and save it at some place, e.g. /var/lib/sss/gpo_cache/ and then the backend will read an process it. This way you already have the file available in the offline case. When calling the child the backend should provide the smb url and a location to store the result. The child can e.g. return a checksum for the file which the backend can save together with the download time in the sysdb cache in a subtree below cn=custom (grep sysdb.h for 'custom' to find the related sysdb calls). With the download time it would be even possible to specific cache lifetime during which the gpo file will not be downloaded again to save bandwidth. But this should be optional.
What do you think?
bye, Sumit _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
With regard to your suggestion to store the policy data in a local file (and store the file's metadata in the cache), I think it would be preferrable to have the child parse the file, and store the relevant data/metadata directly to the cache. I believe this is how IPA HBAC currently works (i.e. storing to cache, rather than to file). This
yes, but the original IPA HBAC data comes from a LDAP tree so storing it to the cache which has LDAP semantics as well is natural.
would allow the file parsing to be done once (when it is written by the child), rather than the file parsing being done every time the policy is processed (when it is read by the backend). Additionally,
That's a good point. Maybe be can do both, the child just reads and saves the file and the backend parses it once and save the access control data to the cache so that it has to parse the file again only when a new version is read by the child.
keep in mind that we are interested in only a subset of the stanzas (i.e. "Privilege Rights" stanza) in the Security Settings policy file (GptTmpl.inf). In other words, only a small portion of the file contains data that is relevant for us.
yes, nevertheless I'd like to keep the file around. E.g. it would help to make debugging easier because we do not have to tell users how to download the file from the server to check to content. Additionally other application might be interested in the content as well (they can even ensure that the version of the file is recent by calling pam_acct_mgmt() which would trigger the access provider in the SSSD backend, I agree that this is not the nicest solution :-).
For this initial patch, I am having the child return the allowed_sids and denied_sids to the backend for further processing. However, in subsequent patches, I am planning on having the child store the relevant policy data to the cache, and then have the backend retrieve the relevant policy data from the cache. In other words, the proposed logic for future patches is:
- if we are online, the backend calls the child (which stores the
parsed data to cache)
- regardless of whether we are online or offline, the backend retrieves
the parsed data from the cache and makes a policy decision.
Please do not read/write form/to the cache from the child. So far we tried to limit the child processes to only do what is blocking and cannot be one in an asynchronous ways. Currently this are mostly Kerberos related operations like getting TGTs or updating DNS with GSS-TSIG.
You cite two reasons to save the policy data as a local file:
- to have the local file available as an artifact for debugging and/or
consumption by other applications. 2) to keep processing in the child to the bare minimum (i.e. the child shouldn't cache the policy data)
The first reason doesn't seem very compelling IMO. It is unclear to me why being able to consult the policy file (which includes lots of extraneous policy data) would be better than simply relying on the debug logs (which contain only the relevant policy data). Also, I am not aware of any applications (other than the sssd-ad provider) that are planning on using this policy data. Do you have particular application(s) in mind? The second reason is more compelling. However, if we have the child retrieve the data, parse the data, and return the parsed data to the backend, then the child would not need to interact with the cache (i.e. only the backend would interact with the cache).
The reason I am pushing back on the suggestion to store the data to a file is that it adds complexity without clear benefits IMO (unless I am missing something). For example, if we agree that the backend should parse the data and store it to cache anyway (at least when there is a new version), why not save the step of writing to a local file, by having the child parse the data, and having the backend store the data to cache. Additionally, exposing the *entire* contents of the policy file for debugging purposes, when we are only using a few fields, seems like it would only confuse users.
What about the third one about other applications. Since SSSD is already doing the work of figuring out which GPOs apply to the host and loading the data why not making the data available to others?
OK. I agree that this is compelling enough to use a file-based approach.
With regard to reducing unnecessary downloads, I am planning on implementing the logic suggested by [MS-GPOL] of comparing the cached gpo version against the current gpo version (by downloading only the GPT.INI file). If the gpo versions are the same, there is no need to download the policy file. Again, this is intended for a subsequent patch.
I think the term 'bandwidth' was not chosen well in my comment. I'm not much concerned by the actual download itself but the general costs including creating the connection, doing Kerberos authentication etc. Please note that Windows client might stay connected to the sysvol and might have some clever mechanism to make it cheap to check GPT.INI every time. I think from our point of view just reading GPT.INI or reading GptTmpl.inf shouldn't make much difference in the general case. So I still would suggest to introduce an SSSD specific timeout. But I won't mind checking GPT.INI as well. So the version of the policy should be written to the cache as well and send to the child so that it first can download GPT.INI and check if the version changed. If the version has not changed it should indicate this to the backend so that it know it does not has to read and parse the policy file (if we agree that the backend should do this :-).
In addition to the GPT.INI check, I agree that adding an SSSD specific timeout could be useful. Additionally, the backend should probably send *all* the guid-filtered GPOs to the child at once (so that the child can create a single smb connection, and re-use it for retrieving all the relevant GPT.INI and GptTmpl.inf files). What do you think?
I think my concern about performance could just be based on my current design of the gpo_child. Currently, for *each* smb_uri, I fork/exec a new gpo_child process, send it the smb_uri, have it create an smb connection, perform the smb retrieval, return some data, and then exit the gpo_child process. If gpo-ldap determines that multiple gpos are applicable, the current design could be problematic, as it requires multiple gpo_child processes to be spawned and killed (i.e. one gpo_child process per smb_uri). I am only using this design b/c that is how the ldap_child and krb5_child seem to operate.
Perhaps a better approach would be: to continue to call the gpo_child separately for each smb_uri, but to only launch a single gpo_child process to handle all the gpos related to a single policy application. This would do away with spawning multiple processes; it would also allow the single gpo_child to create and maintain a single SMB connection, at least for the duration of a single policy application. Does this fit in with the design philosophy of minimalist child processes? Or would it be preferrable to just stay with launching a new child process for each smb_uri?
yes, all needed policy files should be loaded in a single run by a single process.
On a separate topic, I am wondering if we should complete the review of the current patch and push it to master, before I potentially add a file-based approached and/or an enhanced gpo_child design. This would keep the current patch from growing even bigger. Also, the current patch produces correct results, so others could play with it (even if we are planning on changing to a file-based approach later on). What do you think?
I think it's ok. I'm not sure if I can finish the review today, if not I'll be done tomorrow.
bye, Sumit
Regards, Yassir. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Tue, Jun 24, 2014 at 12:03:57PM +0200, Sumit Bose wrote:
On a separate topic, I am wondering if we should complete the review of the current patch and push it to master, before I potentially add a file-based approached and/or an enhanced gpo_child design. This would keep the current patch from growing even bigger. Also, the current patch produces correct results, so others could play with it (even if we are planning on changing to a file-based approach later on). What do you think?
I think it's ok.
Yes, me too, as long as the code is permissive or disabled (for now!), I'm fine.
----- Original Message -----
On Tue, Jun 24, 2014 at 12:03:57PM +0200, Sumit Bose wrote:
On a separate topic, I am wondering if we should complete the review of the current patch and push it to master, before I potentially add a file-based approached and/or an enhanced gpo_child design. This would keep the current patch from growing even bigger. Also, the current patch produces correct results, so others could play with it (even if we are planning on changing to a file-based approach later on). What do you think?
I think it's ok.
Yes, me too, as long as the code is permissive or disabled (for now!), I'm fine. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
I submitted a patch to the list some time ago ("AD: Add support for gpo permissive mode"). It is waiting for code review. Currently (without the patch), access control is always enforced. However, with the patch, access control will only be enforced if the ad_gpo_access_control option is explicitly set to "enforcing" (default is "permissive").
Thanks, Yassir.
On Wed, Jun 25, 2014 at 11:46:41PM -0400, Yassir Elley wrote:
----- Original Message -----
On Tue, Jun 24, 2014 at 12:03:57PM +0200, Sumit Bose wrote:
On a separate topic, I am wondering if we should complete the review of the current patch and push it to master, before I potentially add a file-based approached and/or an enhanced gpo_child design. This would keep the current patch from growing even bigger. Also, the current patch produces correct results, so others could play with it (even if we are planning on changing to a file-based approach later on). What do you think?
I think it's ok.
Yes, me too, as long as the code is permissive or disabled (for now!), I'm fine. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
I submitted a patch to the list some time ago ("AD: Add support for gpo permissive mode"). It is waiting for code review. Currently (without the patch), access control is always enforced. However, with the patch, access control will only be enforced if the ad_gpo_access_control option is explicitly set to "enforcing" (default is "permissive").
Thanks, Yassir.
Ah, you're right, I was under the impression it was already pushed. Sorry about that.
On Wed, Jun 18, 2014 at 01:50:17PM -0400, Yassir Elley wrote:
----- Original Message -----
On Tue, Jun 17, 2014 at 04:44:20PM -0400, Yassir Elley wrote:
----- Original Message -----
----- Original Message -----
On Sun, Jun 15, 2014 at 07:08:55PM -0400, Yassir Elley wrote:
- You suggested using the name of the DC that SSSD is currently
connected to in the smb uri (rather than the domain.name, which will require libsmbclient to perform a DNS resolution). Is there an easy way to get the name of the DC that SSSD is currently connected to? I am having trouble finding it.
In struct ad_gpo_access_state you have a member struct sdap_id_conn_ctx *conn. conn->service->uri is the LDAP uri for the current connection. You can use calls from OpenLDAP or ldb to split it into components, picj the hostname and create the smb uri.
In general the uri should always be available since you read the GPO data from LDAP before doing the smb operations. Nevertheless you can call be_resolve_server_send() to make sure it is set, see e.g. auth_get_server() how to use it.
HTH
bye, Sumit _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
I have attached a revised patch that modifies the smb uri to use the server name rather than the domain name.
Thanks, Yassir. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Oops. Forgot to attach the patch.
Yassir.
Thank you, the patch is working as expected and now uses the hostname to connect to the DC. But please use e.g. ldap_url_parse() from OpenLDAP to split the url and take the hostname from the lud_host member of typedef struct ldap_url_desc. The LDAP url can contain port numbers which would currently cause troubles with your scheme.
As a general comment, please try to split your patches into smaller units. This would help to review them especially to compare multiple versions of a patch.
I thought you had previously indicated a preference for a single patch (with the understanding that this makes it more difficult for the reviewer). Perhaps I misunderstood. In any case, I have attached two patches to this email: the previous patch, and a new patch to address your comment about using ldap_url_parse().
ah, sorry, I think I didn't explain it well. My point was to split the patch into smaller self contained unit, e.g. one patch for the smb operation and a second one for parsing the data and extracting the SID lists. Maybe there are even more opportunities to split the patch. Then each patch get its comments and the changes can be squashed into the patch again.
I have not looked at the child code in details yet, but I would like to suggest a change in the workflow. I think the child should only download the gpo file and save it at some place, e.g. /var/lib/sss/gpo_cache/ and then the backend will read an process it. This way you already have the file available in the offline case. When calling the child the backend should provide the smb url and a location to store the result. The child can e.g. return a checksum for the file which the backend can save together with the download time in the sysdb cache in a subtree below cn=custom (grep sysdb.h for 'custom' to find the related sysdb calls). With the download time it would be even possible to specific cache lifetime during which the gpo file will not be downloaded again to save bandwidth. But this should be optional.
What do you think?
bye, Sumit _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
With regard to your suggestion to store the policy data in a local file (and store the file's metadata in the cache), I think it would be preferrable to have the child parse the file, and store the relevant data/metadata directly to the cache. I believe this is how IPA HBAC currently works (i.e. storing to cache, rather than to file). This would allow the file parsing to be done once (when it is written by the child), rather than the file parsing being done every time the policy is processed (when it is read by the backend). Additionally, keep in mind that we are interested in only a subset of the stanzas (i.e. "Privilege Rights" stanza) in the Security Settings policy file (GptTmpl.inf). In other words, only a small portion of the file contains data that is relevant for us.
For this initial patch, I am having the child return the allowed_sids and denied_sids to the backend for further processing. However, in subsequent patches, I am planning on having the child store the relevant policy data to the cache, and then have the backend retrieve the relevant policy data from the cache. In other words, the proposed logic for future patches is:
- if we are online, the backend calls the child (which stores the parsed data to cache)
- regardless of whether we are online or offline, the backend retrieves the parsed data from the cache and makes a policy decision.
With regard to reducing unnecessary downloads, I am planning on implementing the logic suggested by [MS-GPOL] of comparing the cached gpo version against the current gpo version (by downloading only the GPT.INI file). If the gpo versions are the same, there is no need to download the policy file. Again, this is intended for a subsequent patch.
Regards, Yassir.
From c2047c17b1fc8dc6ab0df337a3a0fa0af9cc22e4 Mon Sep 17 00:00:00 2001 From: Yassir Elley yelley@redhat.com Date: Wed, 18 Jun 2014 13:42:50 -0400 Subject: [PATCH 2/2] Use ldap_url_parse to extract hostname from ldap uri
ACK
From d0e3541cd2c0be7ca6c511bc40dd37fe569c3a29 Mon Sep 17 00:00:00 2001 From: Yassir Elley yelley@redhat.com Date: Wed, 21 May 2014 15:11:36 -0400 Subject: [PATCH 1/2] AD-GPO: Add gpo-smb implementation in gpo_child process
...
+/* == ad_gpo_process_cse_send/recv helpers ================================= */
+static errno_t +create_cse_send_buffer(TALLOC_CTX *mem_ctx,
char *smb_uri,
struct io_buffer **io_buf)
+{
- struct io_buffer *buf;
- size_t rp;
- DEBUG(SSSDBG_TRACE_FUNC, "smb_uri: %s\n", smb_uri);
- DEBUG(SSSDBG_TRACE_FUNC, "strlen(smb_uri): %zu\n", strlen(smb_uri));
- buf = talloc(mem_ctx, struct io_buffer);
- if (buf == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "talloc failed.\n");
return ENOMEM;
- }
- buf->size = 1 * sizeof(uint32_t);
- if (smb_uri) {
buf->size += strlen(smb_uri);
- }
- DEBUG(SSSDBG_TRACE_ALL, "buffer size: %zu\n", buf->size);
- buf->data = talloc_size(buf, buf->size);
- if (buf->data == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "talloc_size failed.\n");
talloc_free(buf);
return ENOMEM;
- }
- rp = 0;
- /* smb_uri */
- if (smb_uri) {
SAFEALIGN_SET_UINT32(&buf->data[rp], strlen(smb_uri), &rp);
safealign_memcpy(&buf->data[rp], smb_uri, strlen(smb_uri), &rp);
you are calling strlen(smb_uri) 4 time. Although it is a highly optimized call it will need some time and since I guess that compilers are not smart enough to optimize this into a single call I would suggest to put the length into variable and calculate it only once.
- } else {
SAFEALIGN_SET_UINT32(&buf->data[rp], 0, &rp);
- }
- *io_buf = buf;
- return EOK;
+}
+static errno_t +parse_gpo_child_response(TALLOC_CTX *mem_ctx,
uint8_t *buf, ssize_t size,
char ***_allowed_sids,
int *_allowed_size,
char ***_denied_sids,
int *_denied_size)
+{
- size_t p = 0;
- uint32_t res;
- int allowed_size = 0;
- int denied_size = 0;
- int i = 0;
- int sid_len = 0;
- char **allowed_sids;
- char *allowed_sid;
- char **denied_sids;
- char *denied_sid;
- /* operation result code */
- SAFEALIGN_COPY_UINT32_CHECK(&res, buf + p, size, &p);
- /* allowed_size */
- SAFEALIGN_COPY_UINT32_CHECK(&allowed_size, buf + p, size, &p);
- DEBUG(SSSDBG_TRACE_FUNC, "child response allowed_size: %d\n", allowed_size);
- allowed_sids = talloc_array(mem_ctx, char *, allowed_size + 1);
Why do you need '+ 1'? If you planned a NULL-terminated array you forgot to set the last element to NULL.
- if (allowed_sids == NULL) {
return ENOMEM;
- }
- for (i = 0; i < allowed_size; i++) {
SAFEALIGN_COPY_UINT32_CHECK(&sid_len, buf + p, size, &p);
if ((p + sid_len ) > size) return EINVAL;
allowed_sid = talloc_size(mem_ctx, sizeof(char) * (sid_len + 1));
if (allowed_sid == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "talloc_size failed.\n");
return ENOMEM;
}
safealign_memcpy(allowed_sid, buf+p, sizeof(char) * sid_len, &p);
allowed_sid[sid_len] = '\0';
allowed_sids[i] = talloc_strdup(allowed_sids, allowed_sid);
if (allowed_sids[i] == NULL) {
return ENOMEM;
}
I think you can simplify this by calling talloc_strndup() to copy the data from the buffer and advance p manually.
- }
- /* denied_size */
- SAFEALIGN_COPY_UINT32_CHECK(&denied_size, buf + p, size, &p);
- DEBUG(SSSDBG_TRACE_FUNC, "child response denied_size: %d\n", denied_size);
- denied_sids = talloc_array(mem_ctx, char *, denied_size + 1);
see above
- if (denied_sids == NULL) {
return ENOMEM;
- }
- for (i = 0; i < denied_size; i++) {
SAFEALIGN_COPY_UINT32_CHECK(&sid_len, buf + p, size, &p);
if ((p + sid_len ) > size) return EINVAL;
denied_sid = talloc_size(mem_ctx, sizeof(char) * (sid_len + 1));
if (denied_sid == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "talloc_size failed.\n");
return ENOMEM;
}
safealign_memcpy(denied_sid, buf+p, sizeof(char) * sid_len, &p);
denied_sid[sid_len] = '\0';
denied_sids[i] = talloc_strdup(denied_sids, denied_sid);
if (denied_sids[i] == NULL) {
return ENOMEM;
}
see above
- }
- *_allowed_size = allowed_size;
- *_allowed_sids = talloc_steal(mem_ctx, allowed_sids);
you already allocated allocated_sids on mem_ctx. Nevertheless I think it would be a good idea to use a tmp_ctx here and free all allocated memory in the case of an error.
- *_denied_size = denied_size;
- *_denied_sids = talloc_steal(mem_ctx, denied_sids);
- return EOK;
+}
...
+static errno_t +unpack_buffer(uint8_t *buf,
size_t size,
struct input_buffer *ibuf)
+{
- size_t p = 0;
- uint32_t len;
- DEBUG(SSSDBG_TRACE_FUNC, "total buffer size: %zu\n", size);
- /* smb_uri size and length */
- SAFEALIGN_COPY_UINT32_CHECK(&len, buf + p, size, &p);
- DEBUG(SSSDBG_TRACE_FUNC, "smb_uri size: %d\n", len);
- if (len) {
if ((p + len ) > size) return EINVAL;
ibuf->smb_uri = talloc_strndup(ibuf, (char *)(buf + p), len);
if (ibuf->smb_uri == NULL) return ENOMEM;
DEBUG(SSSDBG_TRACE_FUNC, "got smb_uri: %s\n", ibuf->smb_uri);
p += len;
- }
If I see is correctly you continue processing even if there is no smb_uri maybe and error should be returned here if len == 0?
- return EOK;
+}
+static errno_t +pack_buffer(struct response *r,
int result,
int allowed_size,
char **allowed_sids,
int denied_size,
char **denied_sids)
+{
- int len = 0;
- size_t p = 0;
- int i;
- int sid_len = 0;
- /* A buffer with the following structure must be created:
* int32_t status of the request (required)
uint32_t
* int32_t allowed_size (required)
* uint8_t[allowed_size] (optional if allowed_size == 0)
the description is a bit misleading, you have a uint32_t sid_size, uint8_t[sid_size] and this allowed_size times. I currently do not have a good idea how to write it better.
* int32_t denied_size (required)
* uint8_t[denied_size] (optional if denied_size == 0)
*/
- DEBUG(SSSDBG_TRACE_FUNC, "entering pack_buffer\n");
- for (i = 0; i < allowed_size; i++) {
len += strlen(allowed_sids[i]);
- }
- for (i = 0; i < denied_size; i++) {
len += strlen(denied_sids[i]);
- }
- r->size = (3 + allowed_size + denied_size) * sizeof(uint32_t) + len;
- DEBUG(SSSDBG_TRACE_FUNC, "response size: %zu\n",r->size);
- r->buf = talloc_array(r, uint8_t, r->size);
- if(r->buf == NULL) {
return ENOMEM;
- }
- DEBUG(SSSDBG_TRACE_FUNC,
"result [%d] allowed_size [%d] denied_size [%d]\n",
result, allowed_size, denied_size);
- /* result */
- SAFEALIGN_SET_UINT32(&r->buf[p], result, &p);
- /* allowed_size */
- SAFEALIGN_SET_UINT32(&r->buf[p], allowed_size, &p);
- /* allowed_sids */
- for (i = 0; i < allowed_size; i++) {
sid_len = strlen(allowed_sids[i]);
SAFEALIGN_SET_UINT32(&r->buf[p], sid_len, &p);
safealign_memcpy(&r->buf[p], allowed_sids[i], sid_len, &p);
- }
- /* denied_size */
- SAFEALIGN_SET_UINT32(&r->buf[p], denied_size, &p);
- /* denied_sids */
- for (i = 0; i < denied_size; i++) {
sid_len = strlen(denied_sids[i]);
SAFEALIGN_SET_UINT32(&r->buf[p], sid_len, &p);
safealign_memcpy(&r->buf[p], denied_sids[i], sid_len, &p);
- }
- return EOK;
+}
...
+int +main(int argc, const char *argv[]) +{
- int opt;
- poptContext pc;
- int debug_fd = -1;
- errno_t ret;
- int result;
- TALLOC_CTX *main_ctx = NULL;
- uint8_t *buf = NULL;
- ssize_t len = 0;
- struct input_buffer *ibuf = NULL;
- struct response *resp = NULL;
- size_t written;
- char **allowed_sids;
- int allowed_size;
- char **denied_sids;
- int denied_size;
- int j;
- struct poptOption long_options[] = {
POPT_AUTOHELP
{"debug-level", 'd', POPT_ARG_INT, &debug_level, 0,
_("Debug level"), NULL},
{"debug-timestamps", 0, POPT_ARG_INT, &debug_timestamps, 0,
_("Add debug timestamps"), NULL},
{"debug-microseconds", 0, POPT_ARG_INT, &debug_microseconds, 0,
_("Show timestamps with microseconds"), NULL},
{"debug-fd", 0, POPT_ARG_INT, &debug_fd, 0,
_("An open file descriptor for the debug logs"), NULL},
POPT_TABLEEND
- };
- /* Set debug level to invalid value so we can decide if -d 0 was used. */
- debug_level = SSSDBG_INVALID;
- pc = poptGetContext(argv[0], argc, argv, long_options, 0);
- while((opt = poptGetNextOpt(pc)) != -1) {
switch(opt) {
default:
fprintf(stderr, "\nInvalid option %s: %s\n\n",
poptBadOption(pc, 0), poptStrerror(opt));
poptPrintUsage(pc, stderr, 0);
_exit(-1);
}
- }
- poptFreeContext(pc);
- DEBUG_INIT(debug_level);
- debug_prg_name = talloc_asprintf(NULL, "[sssd[gpo_child[%d]]]", getpid());
- if (debug_prg_name == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "talloc_asprintf failed.\n");
goto fail;
- }
- if (debug_fd != -1) {
ret = set_debug_file_from_fd(debug_fd);
if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE, "set_debug_file_from_fd failed.\n");
}
- }
- DEBUG(SSSDBG_TRACE_FUNC, "gpo_child started.\n");
- main_ctx = talloc_new(NULL);
- if (main_ctx == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new failed.\n");
talloc_free(discard_const(debug_prg_name));
goto fail;
- }
- talloc_steal(main_ctx, debug_prg_name);
- buf = talloc_size(main_ctx, sizeof(uint8_t)*IN_BUF_SIZE);
- if (buf == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "talloc_size failed.\n");
goto fail;
- }
- ibuf = talloc_zero(main_ctx, struct input_buffer);
- if (ibuf == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "talloc_zero failed.\n");
goto fail;
- }
- DEBUG(SSSDBG_TRACE_FUNC, "context initialized\n");
- errno = 0;
- len = sss_atomic_read_s(STDIN_FILENO, buf, IN_BUF_SIZE);
- if (len == -1) {
ret = errno;
DEBUG(SSSDBG_CRIT_FAILURE, "read failed [%d][%s].\n", ret, strerror(ret));
goto fail;
- }
- close(STDIN_FILENO);
- ret = unpack_buffer(buf, len, ibuf);
- if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE,
"unpack_buffer failed.[%d][%s].\n", ret, strerror(ret));
goto fail;
- }
- DEBUG(SSSDBG_TRACE_FUNC, "processing security settings\n");
- result = process_security_settings_cse(main_ctx,
ibuf->smb_uri,
&allowed_sids,
&allowed_size,
&denied_sids,
&denied_size);
- if (result != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE,
"process_security_settings_cse failed.[%d][%s].\n",
ret, strerror(ret));
goto fail;
- }
- DEBUG(SSSDBG_CRIT_FAILURE, "allowed_size = %d\n", allowed_size);
- for (j= 0; j < allowed_size; j++) {
DEBUG(SSSDBG_CRIT_FAILURE, "allowed_sids[%d] = %s\n", j,
allowed_sids[j]);
- }
- DEBUG(SSSDBG_CRIT_FAILURE, "denied_size = %d\n", denied_size);
- for (j= 0; j < denied_size; j++) {
DEBUG(SSSDBG_CRIT_FAILURE, " denied_sids[%d] = %s\n", j,
denied_sids[j]);
- }
- ret = prepare_response(main_ctx, result, allowed_size, allowed_sids, denied_size, denied_sids, &resp);
- if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE, "prepare_response failed. [%d][%s].\n",
ret, strerror(ret));
goto fail;
- }
- errno = 0;
- DEBUG(SSSDBG_TRACE_FUNC, "resp->size: %zu\n", resp->size);
- written = sss_atomic_write_s(STDOUT_FILENO, resp->buf, resp->size);
- if (written == -1) {
ret = errno;
DEBUG(SSSDBG_CRIT_FAILURE, "write failed [%d][%s].\n", ret,
strerror(ret));
goto fail;
- }
- if (written != resp->size) {
DEBUG(SSSDBG_CRIT_FAILURE, "Expected to write %zu bytes, wrote %zu\n",
resp->size, written);
goto fail;
- }
- DEBUG(SSSDBG_TRACE_FUNC, "gpo_child completed successfully\n");
- close(STDOUT_FILENO);
- talloc_free(main_ctx);
- return EXIT_SUCCESS;
+fail:
- DEBUG(SSSDBG_CRIT_FAILURE, "gpo_child failed!\n");
- close(STDOUT_FILENO);
just a nitpick which needs no fixing. There are 'goto fail' before STDIN_FILENO was closed. So in theory you might want to close this here as well. But since close() will fail if STDIN_FILENO is already closed it is not worth the effort to detect all of this properly since the program exits anyway.
- talloc_free(main_ctx);
- return EXIT_FAILURE;
+}
1.8.5.3
bye, Sumit
----- Original Message -----
On Wed, Jun 18, 2014 at 01:50:17PM -0400, Yassir Elley wrote:
----- Original Message -----
On Tue, Jun 17, 2014 at 04:44:20PM -0400, Yassir Elley wrote:
----- Original Message -----
----- Original Message -----
On Sun, Jun 15, 2014 at 07:08:55PM -0400, Yassir Elley wrote: > > > * You suggested using the name of the DC that SSSD is currently > connected > to in the smb uri (rather than the domain.name, which will > require > libsmbclient to perform a DNS resolution). Is there an easy way > to > get > the > name of the DC that SSSD is currently connected to? I am having > trouble > finding it. >
In struct ad_gpo_access_state you have a member struct sdap_id_conn_ctx *conn. conn->service->uri is the LDAP uri for the current connection. You can use calls from OpenLDAP or ldb to split it into components, picj the hostname and create the smb uri.
In general the uri should always be available since you read the GPO data from LDAP before doing the smb operations. Nevertheless you can call be_resolve_server_send() to make sure it is set, see e.g. auth_get_server() how to use it.
HTH
bye, Sumit _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
I have attached a revised patch that modifies the smb uri to use the server name rather than the domain name.
Thanks, Yassir. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Oops. Forgot to attach the patch.
Yassir.
Thank you, the patch is working as expected and now uses the hostname to connect to the DC. But please use e.g. ldap_url_parse() from OpenLDAP to split the url and take the hostname from the lud_host member of typedef struct ldap_url_desc. The LDAP url can contain port numbers which would currently cause troubles with your scheme.
As a general comment, please try to split your patches into smaller units. This would help to review them especially to compare multiple versions of a patch.
I thought you had previously indicated a preference for a single patch (with the understanding that this makes it more difficult for the reviewer). Perhaps I misunderstood. In any case, I have attached two patches to this email: the previous patch, and a new patch to address your comment about using ldap_url_parse().
ah, sorry, I think I didn't explain it well. My point was to split the patch into smaller self contained unit, e.g. one patch for the smb operation and a second one for parsing the data and extracting the SID lists. Maybe there are even more opportunities to split the patch. Then each patch get its comments and the changes can be squashed into the patch again.
Ah, that makes sense. Thanks.
I have not looked at the child code in details yet, but I would like to suggest a change in the workflow. I think the child should only download the gpo file and save it at some place, e.g. /var/lib/sss/gpo_cache/ and then the backend will read an process it. This way you already have the file available in the offline case. When calling the child the backend should provide the smb url and a location to store the result. The child can e.g. return a checksum for the file which the backend can save together with the download time in the sysdb cache in a subtree below cn=custom (grep sysdb.h for 'custom' to find the related sysdb calls). With the download time it would be even possible to specific cache lifetime during which the gpo file will not be downloaded again to save bandwidth. But this should be optional.
What do you think?
bye, Sumit _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
With regard to your suggestion to store the policy data in a local file (and store the file's metadata in the cache), I think it would be preferrable to have the child parse the file, and store the relevant data/metadata directly to the cache. I believe this is how IPA HBAC currently works (i.e. storing to cache, rather than to file). This would allow the file parsing to be done once (when it is written by the child), rather than the file parsing being done every time the policy is processed (when it is read by the backend). Additionally, keep in mind that we are interested in only a subset of the stanzas (i.e. "Privilege Rights" stanza) in the Security Settings policy file (GptTmpl.inf). In other words, only a small portion of the file contains data that is relevant for us.
For this initial patch, I am having the child return the allowed_sids and denied_sids to the backend for further processing. However, in subsequent patches, I am planning on having the child store the relevant policy data to the cache, and then have the backend retrieve the relevant policy data from the cache. In other words, the proposed logic for future patches is:
- if we are online, the backend calls the child (which stores the parsed
data to cache)
- regardless of whether we are online or offline, the backend retrieves the
parsed data from the cache and makes a policy decision.
With regard to reducing unnecessary downloads, I am planning on implementing the logic suggested by [MS-GPOL] of comparing the cached gpo version against the current gpo version (by downloading only the GPT.INI file). If the gpo versions are the same, there is no need to download the policy file. Again, this is intended for a subsequent patch.
Regards, Yassir.
From c2047c17b1fc8dc6ab0df337a3a0fa0af9cc22e4 Mon Sep 17 00:00:00 2001 From: Yassir Elley yelley@redhat.com Date: Wed, 18 Jun 2014 13:42:50 -0400 Subject: [PATCH 2/2] Use ldap_url_parse to extract hostname from ldap uri
ACK
From d0e3541cd2c0be7ca6c511bc40dd37fe569c3a29 Mon Sep 17 00:00:00 2001 From: Yassir Elley yelley@redhat.com Date: Wed, 21 May 2014 15:11:36 -0400 Subject: [PATCH 1/2] AD-GPO: Add gpo-smb implementation in gpo_child process
...
+/* == ad_gpo_process_cse_send/recv helpers ================================= */
+static errno_t +create_cse_send_buffer(TALLOC_CTX *mem_ctx,
char *smb_uri,
struct io_buffer **io_buf)
+{
- struct io_buffer *buf;
- size_t rp;
- DEBUG(SSSDBG_TRACE_FUNC, "smb_uri: %s\n", smb_uri);
- DEBUG(SSSDBG_TRACE_FUNC, "strlen(smb_uri): %zu\n", strlen(smb_uri));
- buf = talloc(mem_ctx, struct io_buffer);
- if (buf == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "talloc failed.\n");
return ENOMEM;
- }
- buf->size = 1 * sizeof(uint32_t);
- if (smb_uri) {
buf->size += strlen(smb_uri);
- }
- DEBUG(SSSDBG_TRACE_ALL, "buffer size: %zu\n", buf->size);
- buf->data = talloc_size(buf, buf->size);
- if (buf->data == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "talloc_size failed.\n");
talloc_free(buf);
return ENOMEM;
- }
- rp = 0;
- /* smb_uri */
- if (smb_uri) {
SAFEALIGN_SET_UINT32(&buf->data[rp], strlen(smb_uri), &rp);
safealign_memcpy(&buf->data[rp], smb_uri, strlen(smb_uri), &rp);
you are calling strlen(smb_uri) 4 time. Although it is a highly optimized call it will need some time and since I guess that compilers are not smart enough to optimize this into a single call I would suggest to put the length into variable and calculate it only once.
- } else {
SAFEALIGN_SET_UINT32(&buf->data[rp], 0, &rp);
- }
- *io_buf = buf;
- return EOK;
+}
+static errno_t +parse_gpo_child_response(TALLOC_CTX *mem_ctx,
uint8_t *buf, ssize_t size,
char ***_allowed_sids,
int *_allowed_size,
char ***_denied_sids,
int *_denied_size)
+{
- size_t p = 0;
- uint32_t res;
- int allowed_size = 0;
- int denied_size = 0;
- int i = 0;
- int sid_len = 0;
- char **allowed_sids;
- char *allowed_sid;
- char **denied_sids;
- char *denied_sid;
- /* operation result code */
- SAFEALIGN_COPY_UINT32_CHECK(&res, buf + p, size, &p);
- /* allowed_size */
- SAFEALIGN_COPY_UINT32_CHECK(&allowed_size, buf + p, size, &p);
- DEBUG(SSSDBG_TRACE_FUNC, "child response allowed_size: %d\n",
allowed_size);
- allowed_sids = talloc_array(mem_ctx, char *, allowed_size + 1);
Why do you need '+ 1'? If you planned a NULL-terminated array you forgot to set the last element to NULL.
- if (allowed_sids == NULL) {
return ENOMEM;
- }
- for (i = 0; i < allowed_size; i++) {
SAFEALIGN_COPY_UINT32_CHECK(&sid_len, buf + p, size, &p);
if ((p + sid_len ) > size) return EINVAL;
allowed_sid = talloc_size(mem_ctx, sizeof(char) * (sid_len + 1));
if (allowed_sid == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "talloc_size failed.\n");
return ENOMEM;
}
safealign_memcpy(allowed_sid, buf+p, sizeof(char) * sid_len, &p);
allowed_sid[sid_len] = '\0';
allowed_sids[i] = talloc_strdup(allowed_sids, allowed_sid);
if (allowed_sids[i] == NULL) {
return ENOMEM;
}
I think you can simplify this by calling talloc_strndup() to copy the data from the buffer and advance p manually.
- }
- /* denied_size */
- SAFEALIGN_COPY_UINT32_CHECK(&denied_size, buf + p, size, &p);
- DEBUG(SSSDBG_TRACE_FUNC, "child response denied_size: %d\n",
denied_size);
- denied_sids = talloc_array(mem_ctx, char *, denied_size + 1);
see above
- if (denied_sids == NULL) {
return ENOMEM;
- }
- for (i = 0; i < denied_size; i++) {
SAFEALIGN_COPY_UINT32_CHECK(&sid_len, buf + p, size, &p);
if ((p + sid_len ) > size) return EINVAL;
denied_sid = talloc_size(mem_ctx, sizeof(char) * (sid_len + 1));
if (denied_sid == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "talloc_size failed.\n");
return ENOMEM;
}
safealign_memcpy(denied_sid, buf+p, sizeof(char) * sid_len, &p);
denied_sid[sid_len] = '\0';
denied_sids[i] = talloc_strdup(denied_sids, denied_sid);
if (denied_sids[i] == NULL) {
return ENOMEM;
}
see above
- }
- *_allowed_size = allowed_size;
- *_allowed_sids = talloc_steal(mem_ctx, allowed_sids);
you already allocated allocated_sids on mem_ctx. Nevertheless I think it would be a good idea to use a tmp_ctx here and free all allocated memory in the case of an error.
- *_denied_size = denied_size;
- *_denied_sids = talloc_steal(mem_ctx, denied_sids);
- return EOK;
+}
...
+static errno_t +unpack_buffer(uint8_t *buf,
size_t size,
struct input_buffer *ibuf)
+{
- size_t p = 0;
- uint32_t len;
- DEBUG(SSSDBG_TRACE_FUNC, "total buffer size: %zu\n", size);
- /* smb_uri size and length */
- SAFEALIGN_COPY_UINT32_CHECK(&len, buf + p, size, &p);
- DEBUG(SSSDBG_TRACE_FUNC, "smb_uri size: %d\n", len);
- if (len) {
if ((p + len ) > size) return EINVAL;
ibuf->smb_uri = talloc_strndup(ibuf, (char *)(buf + p), len);
if (ibuf->smb_uri == NULL) return ENOMEM;
DEBUG(SSSDBG_TRACE_FUNC, "got smb_uri: %s\n", ibuf->smb_uri);
p += len;
- }
If I see is correctly you continue processing even if there is no smb_uri maybe and error should be returned here if len == 0?
- return EOK;
+}
+static errno_t +pack_buffer(struct response *r,
int result,
int allowed_size,
char **allowed_sids,
int denied_size,
char **denied_sids)
+{
- int len = 0;
- size_t p = 0;
- int i;
- int sid_len = 0;
- /* A buffer with the following structure must be created:
* int32_t status of the request (required)
uint32_t
* int32_t allowed_size (required)
* uint8_t[allowed_size] (optional if allowed_size == 0)
the description is a bit misleading, you have a uint32_t sid_size, uint8_t[sid_size] and this allowed_size times. I currently do not have a good idea how to write it better.
* int32_t denied_size (required)
* uint8_t[denied_size] (optional if denied_size == 0)
*/
- DEBUG(SSSDBG_TRACE_FUNC, "entering pack_buffer\n");
- for (i = 0; i < allowed_size; i++) {
len += strlen(allowed_sids[i]);
- }
- for (i = 0; i < denied_size; i++) {
len += strlen(denied_sids[i]);
- }
- r->size = (3 + allowed_size + denied_size) * sizeof(uint32_t) + len;
- DEBUG(SSSDBG_TRACE_FUNC, "response size: %zu\n",r->size);
- r->buf = talloc_array(r, uint8_t, r->size);
- if(r->buf == NULL) {
return ENOMEM;
- }
- DEBUG(SSSDBG_TRACE_FUNC,
"result [%d] allowed_size [%d] denied_size [%d]\n",
result, allowed_size, denied_size);
- /* result */
- SAFEALIGN_SET_UINT32(&r->buf[p], result, &p);
- /* allowed_size */
- SAFEALIGN_SET_UINT32(&r->buf[p], allowed_size, &p);
- /* allowed_sids */
- for (i = 0; i < allowed_size; i++) {
sid_len = strlen(allowed_sids[i]);
SAFEALIGN_SET_UINT32(&r->buf[p], sid_len, &p);
safealign_memcpy(&r->buf[p], allowed_sids[i], sid_len, &p);
- }
- /* denied_size */
- SAFEALIGN_SET_UINT32(&r->buf[p], denied_size, &p);
- /* denied_sids */
- for (i = 0; i < denied_size; i++) {
sid_len = strlen(denied_sids[i]);
SAFEALIGN_SET_UINT32(&r->buf[p], sid_len, &p);
safealign_memcpy(&r->buf[p], denied_sids[i], sid_len, &p);
- }
- return EOK;
+}
...
+int +main(int argc, const char *argv[]) +{
- int opt;
- poptContext pc;
- int debug_fd = -1;
- errno_t ret;
- int result;
- TALLOC_CTX *main_ctx = NULL;
- uint8_t *buf = NULL;
- ssize_t len = 0;
- struct input_buffer *ibuf = NULL;
- struct response *resp = NULL;
- size_t written;
- char **allowed_sids;
- int allowed_size;
- char **denied_sids;
- int denied_size;
- int j;
- struct poptOption long_options[] = {
POPT_AUTOHELP
{"debug-level", 'd', POPT_ARG_INT, &debug_level, 0,
_("Debug level"), NULL},
{"debug-timestamps", 0, POPT_ARG_INT, &debug_timestamps, 0,
_("Add debug timestamps"), NULL},
{"debug-microseconds", 0, POPT_ARG_INT, &debug_microseconds, 0,
_("Show timestamps with microseconds"), NULL},
{"debug-fd", 0, POPT_ARG_INT, &debug_fd, 0,
_("An open file descriptor for the debug logs"), NULL},
POPT_TABLEEND
- };
- /* Set debug level to invalid value so we can decide if -d 0 was used.
*/
- debug_level = SSSDBG_INVALID;
- pc = poptGetContext(argv[0], argc, argv, long_options, 0);
- while((opt = poptGetNextOpt(pc)) != -1) {
switch(opt) {
default:
fprintf(stderr, "\nInvalid option %s: %s\n\n",
poptBadOption(pc, 0), poptStrerror(opt));
poptPrintUsage(pc, stderr, 0);
_exit(-1);
}
- }
- poptFreeContext(pc);
- DEBUG_INIT(debug_level);
- debug_prg_name = talloc_asprintf(NULL, "[sssd[gpo_child[%d]]]",
getpid());
- if (debug_prg_name == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "talloc_asprintf failed.\n");
goto fail;
- }
- if (debug_fd != -1) {
ret = set_debug_file_from_fd(debug_fd);
if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE, "set_debug_file_from_fd
failed.\n");
}
- }
- DEBUG(SSSDBG_TRACE_FUNC, "gpo_child started.\n");
- main_ctx = talloc_new(NULL);
- if (main_ctx == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new failed.\n");
talloc_free(discard_const(debug_prg_name));
goto fail;
- }
- talloc_steal(main_ctx, debug_prg_name);
- buf = talloc_size(main_ctx, sizeof(uint8_t)*IN_BUF_SIZE);
- if (buf == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "talloc_size failed.\n");
goto fail;
- }
- ibuf = talloc_zero(main_ctx, struct input_buffer);
- if (ibuf == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "talloc_zero failed.\n");
goto fail;
- }
- DEBUG(SSSDBG_TRACE_FUNC, "context initialized\n");
- errno = 0;
- len = sss_atomic_read_s(STDIN_FILENO, buf, IN_BUF_SIZE);
- if (len == -1) {
ret = errno;
DEBUG(SSSDBG_CRIT_FAILURE, "read failed [%d][%s].\n", ret,
strerror(ret));
goto fail;
- }
- close(STDIN_FILENO);
- ret = unpack_buffer(buf, len, ibuf);
- if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE,
"unpack_buffer failed.[%d][%s].\n", ret, strerror(ret));
goto fail;
- }
- DEBUG(SSSDBG_TRACE_FUNC, "processing security settings\n");
- result = process_security_settings_cse(main_ctx,
ibuf->smb_uri,
&allowed_sids,
&allowed_size,
&denied_sids,
&denied_size);
- if (result != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE,
"process_security_settings_cse failed.[%d][%s].\n",
ret, strerror(ret));
goto fail;
- }
- DEBUG(SSSDBG_CRIT_FAILURE, "allowed_size = %d\n", allowed_size);
- for (j= 0; j < allowed_size; j++) {
DEBUG(SSSDBG_CRIT_FAILURE, "allowed_sids[%d] = %s\n", j,
allowed_sids[j]);
- }
- DEBUG(SSSDBG_CRIT_FAILURE, "denied_size = %d\n", denied_size);
- for (j= 0; j < denied_size; j++) {
DEBUG(SSSDBG_CRIT_FAILURE, " denied_sids[%d] = %s\n", j,
denied_sids[j]);
- }
- ret = prepare_response(main_ctx, result, allowed_size, allowed_sids,
denied_size, denied_sids, &resp);
- if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE, "prepare_response failed. [%d][%s].\n",
ret, strerror(ret));
goto fail;
- }
- errno = 0;
- DEBUG(SSSDBG_TRACE_FUNC, "resp->size: %zu\n", resp->size);
- written = sss_atomic_write_s(STDOUT_FILENO, resp->buf, resp->size);
- if (written == -1) {
ret = errno;
DEBUG(SSSDBG_CRIT_FAILURE, "write failed [%d][%s].\n", ret,
strerror(ret));
goto fail;
- }
- if (written != resp->size) {
DEBUG(SSSDBG_CRIT_FAILURE, "Expected to write %zu bytes, wrote
%zu\n",
resp->size, written);
goto fail;
- }
- DEBUG(SSSDBG_TRACE_FUNC, "gpo_child completed successfully\n");
- close(STDOUT_FILENO);
- talloc_free(main_ctx);
- return EXIT_SUCCESS;
+fail:
- DEBUG(SSSDBG_CRIT_FAILURE, "gpo_child failed!\n");
- close(STDOUT_FILENO);
just a nitpick which needs no fixing. There are 'goto fail' before STDIN_FILENO was closed. So in theory you might want to close this here as well. But since close() will fail if STDIN_FILENO is already closed it is not worth the effort to detect all of this properly since the program exits anyway.
- talloc_free(main_ctx);
- return EXIT_FAILURE;
+}
1.8.5.3
bye, Sumit _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
I have attached a revised first patch which addresses the code review comments. I'm not sure if I needed to (since it has already been ACK'ed), but I have also attached an unchanged second patch.
Thanks, Yassir.
On Wed, Jun 25, 2014 at 11:33:47PM -0400, Yassir Elley wrote:
I'm not sure if I needed to (since it has already been ACK'ed), but I have also attached an unchanged second patch.
Usually this is a good practice because the person pushing the code has all the recent patches in one e-mail.
On Wed, Jun 25, 2014 at 11:33:47PM -0400, Yassir Elley wrote:
----- Original Message -----
On Wed, Jun 18, 2014 at 01:50:17PM -0400, Yassir Elley wrote:
----- Original Message -----
On Tue, Jun 17, 2014 at 04:44:20PM -0400, Yassir Elley wrote:
----- Original Message -----
----- Original Message ----- > On Sun, Jun 15, 2014 at 07:08:55PM -0400, Yassir Elley wrote: > > > > > > * You suggested using the name of the DC that SSSD is currently > > connected > > to in the smb uri (rather than the domain.name, which will > > require > > libsmbclient to perform a DNS resolution). Is there an easy way > > to > > get > > the > > name of the DC that SSSD is currently connected to? I am having > > trouble > > finding it. > > > > In struct ad_gpo_access_state you have a member struct > sdap_id_conn_ctx > *conn. conn->service->uri is the LDAP uri for the current > connection. > You can use calls from OpenLDAP or ldb to split it into components, > picj > the hostname and create the smb uri. > > In general the uri should always be available since you read the > GPO > data from LDAP before doing the smb operations. Nevertheless you > can > call be_resolve_server_send() to make sure it is set, see e.g. > auth_get_server() how to use it. > > HTH > > bye, > Sumit > _______________________________________________ > sssd-devel mailing list > sssd-devel@lists.fedorahosted.org > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel >
I have attached a revised patch that modifies the smb uri to use the server name rather than the domain name.
Thanks, Yassir. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Oops. Forgot to attach the patch.
Yassir.
Thank you, the patch is working as expected and now uses the hostname to connect to the DC. But please use e.g. ldap_url_parse() from OpenLDAP to split the url and take the hostname from the lud_host member of typedef struct ldap_url_desc. The LDAP url can contain port numbers which would currently cause troubles with your scheme.
As a general comment, please try to split your patches into smaller units. This would help to review them especially to compare multiple versions of a patch.
I thought you had previously indicated a preference for a single patch (with the understanding that this makes it more difficult for the reviewer). Perhaps I misunderstood. In any case, I have attached two patches to this email: the previous patch, and a new patch to address your comment about using ldap_url_parse().
ah, sorry, I think I didn't explain it well. My point was to split the patch into smaller self contained unit, e.g. one patch for the smb operation and a second one for parsing the data and extracting the SID lists. Maybe there are even more opportunities to split the patch. Then each patch get its comments and the changes can be squashed into the patch again.
Ah, that makes sense. Thanks.
I have not looked at the child code in details yet, but I would like to suggest a change in the workflow. I think the child should only download the gpo file and save it at some place, e.g. /var/lib/sss/gpo_cache/ and then the backend will read an process it. This way you already have the file available in the offline case. When calling the child the backend should provide the smb url and a location to store the result. The child can e.g. return a checksum for the file which the backend can save together with the download time in the sysdb cache in a subtree below cn=custom (grep sysdb.h for 'custom' to find the related sysdb calls). With the download time it would be even possible to specific cache lifetime during which the gpo file will not be downloaded again to save bandwidth. But this should be optional.
What do you think?
bye, Sumit _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
With regard to your suggestion to store the policy data in a local file (and store the file's metadata in the cache), I think it would be preferrable to have the child parse the file, and store the relevant data/metadata directly to the cache. I believe this is how IPA HBAC currently works (i.e. storing to cache, rather than to file). This would allow the file parsing to be done once (when it is written by the child), rather than the file parsing being done every time the policy is processed (when it is read by the backend). Additionally, keep in mind that we are interested in only a subset of the stanzas (i.e. "Privilege Rights" stanza) in the Security Settings policy file (GptTmpl.inf). In other words, only a small portion of the file contains data that is relevant for us.
For this initial patch, I am having the child return the allowed_sids and denied_sids to the backend for further processing. However, in subsequent patches, I am planning on having the child store the relevant policy data to the cache, and then have the backend retrieve the relevant policy data from the cache. In other words, the proposed logic for future patches is:
- if we are online, the backend calls the child (which stores the parsed
data to cache)
- regardless of whether we are online or offline, the backend retrieves the
parsed data from the cache and makes a policy decision.
With regard to reducing unnecessary downloads, I am planning on implementing the logic suggested by [MS-GPOL] of comparing the cached gpo version against the current gpo version (by downloading only the GPT.INI file). If the gpo versions are the same, there is no need to download the policy file. Again, this is intended for a subsequent patch.
Regards, Yassir.
From c2047c17b1fc8dc6ab0df337a3a0fa0af9cc22e4 Mon Sep 17 00:00:00 2001 From: Yassir Elley yelley@redhat.com Date: Wed, 18 Jun 2014 13:42:50 -0400 Subject: [PATCH 2/2] Use ldap_url_parse to extract hostname from ldap uri
ACK
From d0e3541cd2c0be7ca6c511bc40dd37fe569c3a29 Mon Sep 17 00:00:00 2001 From: Yassir Elley yelley@redhat.com Date: Wed, 21 May 2014 15:11:36 -0400 Subject: [PATCH 1/2] AD-GPO: Add gpo-smb implementation in gpo_child process
...
+/* == ad_gpo_process_cse_send/recv helpers ================================= */
+static errno_t +create_cse_send_buffer(TALLOC_CTX *mem_ctx,
char *smb_uri,
struct io_buffer **io_buf)
+{
- struct io_buffer *buf;
- size_t rp;
- DEBUG(SSSDBG_TRACE_FUNC, "smb_uri: %s\n", smb_uri);
- DEBUG(SSSDBG_TRACE_FUNC, "strlen(smb_uri): %zu\n", strlen(smb_uri));
- buf = talloc(mem_ctx, struct io_buffer);
- if (buf == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "talloc failed.\n");
return ENOMEM;
- }
- buf->size = 1 * sizeof(uint32_t);
- if (smb_uri) {
buf->size += strlen(smb_uri);
- }
- DEBUG(SSSDBG_TRACE_ALL, "buffer size: %zu\n", buf->size);
- buf->data = talloc_size(buf, buf->size);
- if (buf->data == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "talloc_size failed.\n");
talloc_free(buf);
return ENOMEM;
- }
- rp = 0;
- /* smb_uri */
- if (smb_uri) {
SAFEALIGN_SET_UINT32(&buf->data[rp], strlen(smb_uri), &rp);
safealign_memcpy(&buf->data[rp], smb_uri, strlen(smb_uri), &rp);
you are calling strlen(smb_uri) 4 time. Although it is a highly optimized call it will need some time and since I guess that compilers are not smart enough to optimize this into a single call I would suggest to put the length into variable and calculate it only once.
- } else {
SAFEALIGN_SET_UINT32(&buf->data[rp], 0, &rp);
- }
- *io_buf = buf;
- return EOK;
+}
+static errno_t +parse_gpo_child_response(TALLOC_CTX *mem_ctx,
uint8_t *buf, ssize_t size,
char ***_allowed_sids,
int *_allowed_size,
char ***_denied_sids,
int *_denied_size)
+{
- size_t p = 0;
- uint32_t res;
- int allowed_size = 0;
- int denied_size = 0;
- int i = 0;
- int sid_len = 0;
- char **allowed_sids;
- char *allowed_sid;
- char **denied_sids;
- char *denied_sid;
- /* operation result code */
- SAFEALIGN_COPY_UINT32_CHECK(&res, buf + p, size, &p);
- /* allowed_size */
- SAFEALIGN_COPY_UINT32_CHECK(&allowed_size, buf + p, size, &p);
- DEBUG(SSSDBG_TRACE_FUNC, "child response allowed_size: %d\n",
allowed_size);
- allowed_sids = talloc_array(mem_ctx, char *, allowed_size + 1);
Why do you need '+ 1'? If you planned a NULL-terminated array you forgot to set the last element to NULL.
- if (allowed_sids == NULL) {
return ENOMEM;
- }
- for (i = 0; i < allowed_size; i++) {
SAFEALIGN_COPY_UINT32_CHECK(&sid_len, buf + p, size, &p);
if ((p + sid_len ) > size) return EINVAL;
allowed_sid = talloc_size(mem_ctx, sizeof(char) * (sid_len + 1));
if (allowed_sid == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "talloc_size failed.\n");
return ENOMEM;
}
safealign_memcpy(allowed_sid, buf+p, sizeof(char) * sid_len, &p);
allowed_sid[sid_len] = '\0';
allowed_sids[i] = talloc_strdup(allowed_sids, allowed_sid);
if (allowed_sids[i] == NULL) {
return ENOMEM;
}
I think you can simplify this by calling talloc_strndup() to copy the data from the buffer and advance p manually.
- }
- /* denied_size */
- SAFEALIGN_COPY_UINT32_CHECK(&denied_size, buf + p, size, &p);
- DEBUG(SSSDBG_TRACE_FUNC, "child response denied_size: %d\n",
denied_size);
- denied_sids = talloc_array(mem_ctx, char *, denied_size + 1);
see above
- if (denied_sids == NULL) {
return ENOMEM;
- }
- for (i = 0; i < denied_size; i++) {
SAFEALIGN_COPY_UINT32_CHECK(&sid_len, buf + p, size, &p);
if ((p + sid_len ) > size) return EINVAL;
denied_sid = talloc_size(mem_ctx, sizeof(char) * (sid_len + 1));
if (denied_sid == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "talloc_size failed.\n");
return ENOMEM;
}
safealign_memcpy(denied_sid, buf+p, sizeof(char) * sid_len, &p);
denied_sid[sid_len] = '\0';
denied_sids[i] = talloc_strdup(denied_sids, denied_sid);
if (denied_sids[i] == NULL) {
return ENOMEM;
}
see above
- }
- *_allowed_size = allowed_size;
- *_allowed_sids = talloc_steal(mem_ctx, allowed_sids);
you already allocated allocated_sids on mem_ctx. Nevertheless I think it would be a good idea to use a tmp_ctx here and free all allocated memory in the case of an error.
- *_denied_size = denied_size;
- *_denied_sids = talloc_steal(mem_ctx, denied_sids);
- return EOK;
+}
...
+static errno_t +unpack_buffer(uint8_t *buf,
size_t size,
struct input_buffer *ibuf)
+{
- size_t p = 0;
- uint32_t len;
- DEBUG(SSSDBG_TRACE_FUNC, "total buffer size: %zu\n", size);
- /* smb_uri size and length */
- SAFEALIGN_COPY_UINT32_CHECK(&len, buf + p, size, &p);
- DEBUG(SSSDBG_TRACE_FUNC, "smb_uri size: %d\n", len);
- if (len) {
if ((p + len ) > size) return EINVAL;
ibuf->smb_uri = talloc_strndup(ibuf, (char *)(buf + p), len);
if (ibuf->smb_uri == NULL) return ENOMEM;
DEBUG(SSSDBG_TRACE_FUNC, "got smb_uri: %s\n", ibuf->smb_uri);
p += len;
- }
If I see is correctly you continue processing even if there is no smb_uri maybe and error should be returned here if len == 0?
- return EOK;
+}
+static errno_t +pack_buffer(struct response *r,
int result,
int allowed_size,
char **allowed_sids,
int denied_size,
char **denied_sids)
+{
- int len = 0;
- size_t p = 0;
- int i;
- int sid_len = 0;
- /* A buffer with the following structure must be created:
* int32_t status of the request (required)
uint32_t
* int32_t allowed_size (required)
* uint8_t[allowed_size] (optional if allowed_size == 0)
the description is a bit misleading, you have a uint32_t sid_size, uint8_t[sid_size] and this allowed_size times. I currently do not have a good idea how to write it better.
* int32_t denied_size (required)
* uint8_t[denied_size] (optional if denied_size == 0)
*/
- DEBUG(SSSDBG_TRACE_FUNC, "entering pack_buffer\n");
- for (i = 0; i < allowed_size; i++) {
len += strlen(allowed_sids[i]);
- }
- for (i = 0; i < denied_size; i++) {
len += strlen(denied_sids[i]);
- }
- r->size = (3 + allowed_size + denied_size) * sizeof(uint32_t) + len;
- DEBUG(SSSDBG_TRACE_FUNC, "response size: %zu\n",r->size);
- r->buf = talloc_array(r, uint8_t, r->size);
- if(r->buf == NULL) {
return ENOMEM;
- }
- DEBUG(SSSDBG_TRACE_FUNC,
"result [%d] allowed_size [%d] denied_size [%d]\n",
result, allowed_size, denied_size);
- /* result */
- SAFEALIGN_SET_UINT32(&r->buf[p], result, &p);
- /* allowed_size */
- SAFEALIGN_SET_UINT32(&r->buf[p], allowed_size, &p);
- /* allowed_sids */
- for (i = 0; i < allowed_size; i++) {
sid_len = strlen(allowed_sids[i]);
SAFEALIGN_SET_UINT32(&r->buf[p], sid_len, &p);
safealign_memcpy(&r->buf[p], allowed_sids[i], sid_len, &p);
- }
- /* denied_size */
- SAFEALIGN_SET_UINT32(&r->buf[p], denied_size, &p);
- /* denied_sids */
- for (i = 0; i < denied_size; i++) {
sid_len = strlen(denied_sids[i]);
SAFEALIGN_SET_UINT32(&r->buf[p], sid_len, &p);
safealign_memcpy(&r->buf[p], denied_sids[i], sid_len, &p);
- }
- return EOK;
+}
...
+int +main(int argc, const char *argv[]) +{
- int opt;
- poptContext pc;
- int debug_fd = -1;
- errno_t ret;
- int result;
- TALLOC_CTX *main_ctx = NULL;
- uint8_t *buf = NULL;
- ssize_t len = 0;
- struct input_buffer *ibuf = NULL;
- struct response *resp = NULL;
- size_t written;
- char **allowed_sids;
- int allowed_size;
- char **denied_sids;
- int denied_size;
- int j;
- struct poptOption long_options[] = {
POPT_AUTOHELP
{"debug-level", 'd', POPT_ARG_INT, &debug_level, 0,
_("Debug level"), NULL},
{"debug-timestamps", 0, POPT_ARG_INT, &debug_timestamps, 0,
_("Add debug timestamps"), NULL},
{"debug-microseconds", 0, POPT_ARG_INT, &debug_microseconds, 0,
_("Show timestamps with microseconds"), NULL},
{"debug-fd", 0, POPT_ARG_INT, &debug_fd, 0,
_("An open file descriptor for the debug logs"), NULL},
POPT_TABLEEND
- };
- /* Set debug level to invalid value so we can decide if -d 0 was used.
*/
- debug_level = SSSDBG_INVALID;
- pc = poptGetContext(argv[0], argc, argv, long_options, 0);
- while((opt = poptGetNextOpt(pc)) != -1) {
switch(opt) {
default:
fprintf(stderr, "\nInvalid option %s: %s\n\n",
poptBadOption(pc, 0), poptStrerror(opt));
poptPrintUsage(pc, stderr, 0);
_exit(-1);
}
- }
- poptFreeContext(pc);
- DEBUG_INIT(debug_level);
- debug_prg_name = talloc_asprintf(NULL, "[sssd[gpo_child[%d]]]",
getpid());
- if (debug_prg_name == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "talloc_asprintf failed.\n");
goto fail;
- }
- if (debug_fd != -1) {
ret = set_debug_file_from_fd(debug_fd);
if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE, "set_debug_file_from_fd
failed.\n");
}
- }
- DEBUG(SSSDBG_TRACE_FUNC, "gpo_child started.\n");
- main_ctx = talloc_new(NULL);
- if (main_ctx == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new failed.\n");
talloc_free(discard_const(debug_prg_name));
goto fail;
- }
- talloc_steal(main_ctx, debug_prg_name);
- buf = talloc_size(main_ctx, sizeof(uint8_t)*IN_BUF_SIZE);
- if (buf == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "talloc_size failed.\n");
goto fail;
- }
- ibuf = talloc_zero(main_ctx, struct input_buffer);
- if (ibuf == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "talloc_zero failed.\n");
goto fail;
- }
- DEBUG(SSSDBG_TRACE_FUNC, "context initialized\n");
- errno = 0;
- len = sss_atomic_read_s(STDIN_FILENO, buf, IN_BUF_SIZE);
- if (len == -1) {
ret = errno;
DEBUG(SSSDBG_CRIT_FAILURE, "read failed [%d][%s].\n", ret,
strerror(ret));
goto fail;
- }
- close(STDIN_FILENO);
- ret = unpack_buffer(buf, len, ibuf);
- if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE,
"unpack_buffer failed.[%d][%s].\n", ret, strerror(ret));
goto fail;
- }
- DEBUG(SSSDBG_TRACE_FUNC, "processing security settings\n");
- result = process_security_settings_cse(main_ctx,
ibuf->smb_uri,
&allowed_sids,
&allowed_size,
&denied_sids,
&denied_size);
- if (result != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE,
"process_security_settings_cse failed.[%d][%s].\n",
ret, strerror(ret));
goto fail;
- }
- DEBUG(SSSDBG_CRIT_FAILURE, "allowed_size = %d\n", allowed_size);
- for (j= 0; j < allowed_size; j++) {
DEBUG(SSSDBG_CRIT_FAILURE, "allowed_sids[%d] = %s\n", j,
allowed_sids[j]);
- }
- DEBUG(SSSDBG_CRIT_FAILURE, "denied_size = %d\n", denied_size);
- for (j= 0; j < denied_size; j++) {
DEBUG(SSSDBG_CRIT_FAILURE, " denied_sids[%d] = %s\n", j,
denied_sids[j]);
- }
- ret = prepare_response(main_ctx, result, allowed_size, allowed_sids,
denied_size, denied_sids, &resp);
- if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE, "prepare_response failed. [%d][%s].\n",
ret, strerror(ret));
goto fail;
- }
- errno = 0;
- DEBUG(SSSDBG_TRACE_FUNC, "resp->size: %zu\n", resp->size);
- written = sss_atomic_write_s(STDOUT_FILENO, resp->buf, resp->size);
- if (written == -1) {
ret = errno;
DEBUG(SSSDBG_CRIT_FAILURE, "write failed [%d][%s].\n", ret,
strerror(ret));
goto fail;
- }
- if (written != resp->size) {
DEBUG(SSSDBG_CRIT_FAILURE, "Expected to write %zu bytes, wrote
%zu\n",
resp->size, written);
goto fail;
- }
- DEBUG(SSSDBG_TRACE_FUNC, "gpo_child completed successfully\n");
- close(STDOUT_FILENO);
- talloc_free(main_ctx);
- return EXIT_SUCCESS;
+fail:
- DEBUG(SSSDBG_CRIT_FAILURE, "gpo_child failed!\n");
- close(STDOUT_FILENO);
just a nitpick which needs no fixing. There are 'goto fail' before STDIN_FILENO was closed. So in theory you might want to close this here as well. But since close() will fail if STDIN_FILENO is already closed it is not worth the effort to detect all of this properly since the program exits anyway.
- talloc_free(main_ctx);
- return EXIT_FAILURE;
+}
1.8.5.3
bye, Sumit _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
I have attached a revised first patch which addresses the code review comments. I'm not sure if I needed to (since it has already been ACK'ed), but I have also attached an unchanged second patch.
ACK.
I will now look at the permissive mode patch.
bye, Sumit
Thanks, Yassir.
On (25/06/14 23:33), Yassir Elley wrote:
I have attached a revised first patch which addresses the code review comments. I'm not sure if I needed to (since it has already been ACK'ed), but I have also attached an unchanged second patch.
Thanks, Yassir.
I know that Sumit already ACKed these patches, but I used them to test CI script and I found some minor issues + warnings from static analysers (as usual :-)
From b58f39645872bc1dd878ac650861af89964101f9 Mon Sep 17 00:00:00 2001 From: Yassir Elley yelley@redhat.com Date: Wed, 21 May 2014 15:11:36 -0400 Subject: [PATCH 1/2] AD-GPO: Add gpo-smb implementation in gpo_child process
Makefile.am | 22 + contrib/sssd.spec.in | 2 +- src/external/samba.m4 | 8 + src/providers/ad/ad_gpo.c | 887 +++++++++++++++++++++++++++++++++++++++- src/providers/ad/ad_gpo_child.c | 637 +++++++++++++++++++++++++++++ 5 files changed, 1543 insertions(+), 13 deletions(-) create mode 100644 src/providers/ad/ad_gpo_child.c
diff --git a/Makefile.am b/Makefile.am index 83999f3584fe130320ebfa6ea459f5706992c6a4..6cf5d8cb0c7c68314dad49461e7cd1fb4452bc1b 100644 --- a/Makefile.am +++ b/Makefile.am @@ -121,6 +121,9 @@ endif if BUILD_IFP sssdlibexec_PROGRAMS += sssd_ifp endif +if BUILD_SAMBA +sssdlibexec_PROGRAMS += gpo_child +endif
if BUILD_PAC_RESPONDER @@ -2298,6 +2301,25 @@ ldap_child_LDADD = \ $(DHASH_LIBS) \ $(KRB5_LIBS)
+gpo_child_SOURCES = \
- src/providers/ad/ad_gpo_child.c \
- src/util/atomic_io.c \
- src/util/util.c \
- src/util/signal.c
+gpo_child_CFLAGS = \
- $(AM_CFLAGS) \
- $(POPT_CFLAGS) \
- $(KRB5_CFLAGS) \
- $(INI_CONFIG_CFLAGS) \
- $(SMBCLIENT_CFLAGS)
+gpo_child_LDADD = \
- libsss_debug.la \
- $(TALLOC_LIBS) \
- $(POPT_LIBS) \
- $(DHASH_LIBS) \
- $(INI_CONFIG_LIBS) \
- $(SMBCLIENT_LIBS)
proxy_child_SOURCES = \ src/providers/proxy/proxy_child.c \ src/providers/data_provider_iface_generated.c \ diff --git a/contrib/sssd.spec.in b/contrib/sssd.spec.in index b5e7903713ccc6944fbed4f84c2f0413a257b760..10704ae48cf5636de2a5cbbb42fc00b99052144f 100644 --- a/contrib/sssd.spec.in +++ b/contrib/sssd.spec.in @@ -97,7 +97,7 @@ BuildRequires: libtdb-devel BuildRequires: libldb-devel BuildRequires: libdhash-devel >= 0.4.2 BuildRequires: libcollection-devel -BuildRequires: libini_config-devel +BuildRequires: libini_config-devel >= 1.1 BuildRequires: dbus-devel BuildRequires: dbus-libs %if 0%{?rhel5_minor} >= 7 diff --git a/src/external/samba.m4 b/src/external/samba.m4 index 735cc5a187dba7514a9c7436a6d3ba948ad522b0..bf790365468da5ccdd40a260d74893490b7e7be8 100644 --- a/src/external/samba.m4 +++ b/src/external/samba.m4 @@ -19,4 +19,12 @@ If you do not want to build these providers it is possible to build SSSD without them. In this case, you will need to execute configure script with argument --without-samba ]]))
- PKG_CHECK_MODULES(INI_CONFIG, ini_config >= 1.0.0, ,
^^^^^ 1.1.0 You used correct version in spec file. configure will pass with old version of ding-libs and compilation fail in gpo_child.
AC_MSG_ERROR([[Please install libini_config development libraries.
+libini_config libraries are necessary for building ad and ipa provider.
^^^^^^^^^ You touch this code. I might be good to slightly modify text. "ad with gpo" or better wording. Sorry, for nitpicking. I would ignore it there was not problem with version.
+If you do not want to build these providers it is possible to build SSSD +without them. In this case, you will need to execute configure script +with argument --without-samba
- ]]))
fi
//snip
+/* == ad_gpo_process_cse_send/recv helpers ================================= */
+static errno_t +create_cse_send_buffer(TALLOC_CTX *mem_ctx,
char *smb_uri,
struct io_buffer **io_buf)
+{
- struct io_buffer *buf;
- size_t rp;
- int smb_uri_length;
- smb_uri_length = strlen(smb_uri);
- DEBUG(SSSDBG_TRACE_FUNC, "smb_uri: %s\n", smb_uri);
- DEBUG(SSSDBG_TRACE_FUNC, "strlen(smb_uri): %d\n", smb_uri_length);
- buf = talloc(mem_ctx, struct io_buffer);
- if (buf == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "talloc failed.\n");
return ENOMEM;
- }
- buf->size = 1 * sizeof(uint32_t);
- if (smb_uri) {
I thing that this test is redundant, because strlen would crash anyway with NULL.
Error: REVERSE_INULL sssd-1.11.92/src/providers/ad/ad_gpo.c:2792: deref_ptr_in_call: Dereferencing pointer "smb_uri". sssd-1.11.92/src/providers/ad/ad_gpo.c:2804: check_after_deref: Null-checking "smb_uri" suggests that it may be null, but it has already been dereferenced on all paths leading to the check.
buf->size += smb_uri_length;
- }
- DEBUG(SSSDBG_TRACE_ALL, "buffer size: %zu\n", buf->size);
- buf->data = talloc_size(buf, buf->size);
- if (buf->data == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "talloc_size failed.\n");
talloc_free(buf);
return ENOMEM;
- }
- rp = 0;
- /* smb_uri */
- if (smb_uri) {
SAFEALIGN_SET_UINT32(&buf->data[rp], smb_uri_length, &rp);
safealign_memcpy(&buf->data[rp], smb_uri, smb_uri_length, &rp);
- } else {
SAFEALIGN_SET_UINT32(&buf->data[rp], 0, &rp);
- }
- *io_buf = buf;
- return EOK;
+}
I am attaching another html report from clang.
I let final ACK for Sumit.
LS
----- Original Message -----
On (25/06/14 23:33), Yassir Elley wrote:
I have attached a revised first patch which addresses the code review comments. I'm not sure if I needed to (since it has already been ACK'ed), but I have also attached an unchanged second patch.
Thanks, Yassir.
I know that Sumit already ACKed these patches, but I used them to test CI script and I found some minor issues
- warnings from static analysers (as usual :-)
From b58f39645872bc1dd878ac650861af89964101f9 Mon Sep 17 00:00:00 2001 From: Yassir Elley yelley@redhat.com Date: Wed, 21 May 2014 15:11:36 -0400 Subject: [PATCH 1/2] AD-GPO: Add gpo-smb implementation in gpo_child process
Makefile.am | 22 + contrib/sssd.spec.in | 2 +- src/external/samba.m4 | 8 + src/providers/ad/ad_gpo.c | 887 +++++++++++++++++++++++++++++++++++++++- src/providers/ad/ad_gpo_child.c | 637 +++++++++++++++++++++++++++++ 5 files changed, 1543 insertions(+), 13 deletions(-) create mode 100644 src/providers/ad/ad_gpo_child.c
diff --git a/Makefile.am b/Makefile.am index 83999f3584fe130320ebfa6ea459f5706992c6a4..6cf5d8cb0c7c68314dad49461e7cd1fb4452bc1b 100644 --- a/Makefile.am +++ b/Makefile.am @@ -121,6 +121,9 @@ endif if BUILD_IFP sssdlibexec_PROGRAMS += sssd_ifp endif +if BUILD_SAMBA +sssdlibexec_PROGRAMS += gpo_child +endif
if BUILD_PAC_RESPONDER @@ -2298,6 +2301,25 @@ ldap_child_LDADD = \ $(DHASH_LIBS) \ $(KRB5_LIBS)
+gpo_child_SOURCES = \
- src/providers/ad/ad_gpo_child.c \
- src/util/atomic_io.c \
- src/util/util.c \
- src/util/signal.c
+gpo_child_CFLAGS = \
- $(AM_CFLAGS) \
- $(POPT_CFLAGS) \
- $(KRB5_CFLAGS) \
- $(INI_CONFIG_CFLAGS) \
- $(SMBCLIENT_CFLAGS)
+gpo_child_LDADD = \
- libsss_debug.la \
- $(TALLOC_LIBS) \
- $(POPT_LIBS) \
- $(DHASH_LIBS) \
- $(INI_CONFIG_LIBS) \
- $(SMBCLIENT_LIBS)
proxy_child_SOURCES = \ src/providers/proxy/proxy_child.c \ src/providers/data_provider_iface_generated.c \ diff --git a/contrib/sssd.spec.in b/contrib/sssd.spec.in index b5e7903713ccc6944fbed4f84c2f0413a257b760..10704ae48cf5636de2a5cbbb42fc00b99052144f 100644 --- a/contrib/sssd.spec.in +++ b/contrib/sssd.spec.in @@ -97,7 +97,7 @@ BuildRequires: libtdb-devel BuildRequires: libldb-devel BuildRequires: libdhash-devel >= 0.4.2 BuildRequires: libcollection-devel -BuildRequires: libini_config-devel +BuildRequires: libini_config-devel >= 1.1 BuildRequires: dbus-devel BuildRequires: dbus-libs %if 0%{?rhel5_minor} >= 7 diff --git a/src/external/samba.m4 b/src/external/samba.m4 index 735cc5a187dba7514a9c7436a6d3ba948ad522b0..bf790365468da5ccdd40a260d74893490b7e7be8 100644 --- a/src/external/samba.m4 +++ b/src/external/samba.m4 @@ -19,4 +19,12 @@ If you do not want to build these providers it is possible to build SSSD without them. In this case, you will need to execute configure script with argument --without-samba ]]))
- PKG_CHECK_MODULES(INI_CONFIG, ini_config >= 1.0.0, ,
^^^^^ 1.1.0 You used correct version in spec file.
configure will pass with old version of ding-libs and compilation fail in gpo_child.
AC_MSG_ERROR([[Please install libini_config development libraries.
+libini_config libraries are necessary for building ad and ipa provider.
^^^^^^^^^ You touch this code. I might be good to slightly modify text. "ad with gpo" or better wording.
Sorry, for nitpicking. I would ignore it there was not problem with version.
+If you do not want to build these providers it is possible to build SSSD +without them. In this case, you will need to execute configure script +with argument --without-samba
- ]]))
fi
//snip
+/* == ad_gpo_process_cse_send/recv helpers ================================= */
+static errno_t +create_cse_send_buffer(TALLOC_CTX *mem_ctx,
char *smb_uri,
struct io_buffer **io_buf)
+{
- struct io_buffer *buf;
- size_t rp;
- int smb_uri_length;
- smb_uri_length = strlen(smb_uri);
- DEBUG(SSSDBG_TRACE_FUNC, "smb_uri: %s\n", smb_uri);
- DEBUG(SSSDBG_TRACE_FUNC, "strlen(smb_uri): %d\n", smb_uri_length);
- buf = talloc(mem_ctx, struct io_buffer);
- if (buf == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "talloc failed.\n");
return ENOMEM;
- }
- buf->size = 1 * sizeof(uint32_t);
- if (smb_uri) {
I thing that this test is redundant, because strlen would crash anyway with NULL.
Error: REVERSE_INULL sssd-1.11.92/src/providers/ad/ad_gpo.c:2792: deref_ptr_in_call: Dereferencing pointer "smb_uri". sssd-1.11.92/src/providers/ad/ad_gpo.c:2804: check_after_deref: Null-checking "smb_uri" suggests that it may be null, but it has already been dereferenced on all paths leading to the check.
buf->size += smb_uri_length;
- }
- DEBUG(SSSDBG_TRACE_ALL, "buffer size: %zu\n", buf->size);
- buf->data = talloc_size(buf, buf->size);
- if (buf->data == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "talloc_size failed.\n");
talloc_free(buf);
return ENOMEM;
- }
- rp = 0;
- /* smb_uri */
- if (smb_uri) {
SAFEALIGN_SET_UINT32(&buf->data[rp], smb_uri_length, &rp);
safealign_memcpy(&buf->data[rp], smb_uri, smb_uri_length, &rp);
- } else {
SAFEALIGN_SET_UINT32(&buf->data[rp], 0, &rp);
- }
- *io_buf = buf;
- return EOK;
+}
I am attaching another html report from clang.
I let final ACK for Sumit.
LS
Revised patches attached.
Regards, Yassir.
On Sat, Jun 28, 2014 at 01:30:36AM -0400, Yassir Elley wrote:
----- Original Message -----
On (25/06/14 23:33), Yassir Elley wrote:
I have attached a revised first patch which addresses the code review comments. I'm not sure if I needed to (since it has already been ACK'ed), but I have also attached an unchanged second patch.
Thanks, Yassir.
I know that Sumit already ACKed these patches, but I used them to test CI script and I found some minor issues
- warnings from static analysers (as usual :-)
From b58f39645872bc1dd878ac650861af89964101f9 Mon Sep 17 00:00:00 2001 From: Yassir Elley yelley@redhat.com Date: Wed, 21 May 2014 15:11:36 -0400 Subject: [PATCH 1/2] AD-GPO: Add gpo-smb implementation in gpo_child process
Makefile.am | 22 + contrib/sssd.spec.in | 2 +- src/external/samba.m4 | 8 + src/providers/ad/ad_gpo.c | 887 +++++++++++++++++++++++++++++++++++++++- src/providers/ad/ad_gpo_child.c | 637 +++++++++++++++++++++++++++++ 5 files changed, 1543 insertions(+), 13 deletions(-) create mode 100644 src/providers/ad/ad_gpo_child.c
diff --git a/Makefile.am b/Makefile.am index 83999f3584fe130320ebfa6ea459f5706992c6a4..6cf5d8cb0c7c68314dad49461e7cd1fb4452bc1b 100644 --- a/Makefile.am +++ b/Makefile.am @@ -121,6 +121,9 @@ endif if BUILD_IFP sssdlibexec_PROGRAMS += sssd_ifp endif +if BUILD_SAMBA +sssdlibexec_PROGRAMS += gpo_child +endif
if BUILD_PAC_RESPONDER @@ -2298,6 +2301,25 @@ ldap_child_LDADD = \ $(DHASH_LIBS) \ $(KRB5_LIBS)
+gpo_child_SOURCES = \
- src/providers/ad/ad_gpo_child.c \
- src/util/atomic_io.c \
- src/util/util.c \
- src/util/signal.c
+gpo_child_CFLAGS = \
- $(AM_CFLAGS) \
- $(POPT_CFLAGS) \
- $(KRB5_CFLAGS) \
- $(INI_CONFIG_CFLAGS) \
- $(SMBCLIENT_CFLAGS)
+gpo_child_LDADD = \
- libsss_debug.la \
- $(TALLOC_LIBS) \
- $(POPT_LIBS) \
- $(DHASH_LIBS) \
- $(INI_CONFIG_LIBS) \
- $(SMBCLIENT_LIBS)
proxy_child_SOURCES = \ src/providers/proxy/proxy_child.c \ src/providers/data_provider_iface_generated.c \ diff --git a/contrib/sssd.spec.in b/contrib/sssd.spec.in index b5e7903713ccc6944fbed4f84c2f0413a257b760..10704ae48cf5636de2a5cbbb42fc00b99052144f 100644 --- a/contrib/sssd.spec.in +++ b/contrib/sssd.spec.in @@ -97,7 +97,7 @@ BuildRequires: libtdb-devel BuildRequires: libldb-devel BuildRequires: libdhash-devel >= 0.4.2 BuildRequires: libcollection-devel -BuildRequires: libini_config-devel +BuildRequires: libini_config-devel >= 1.1 BuildRequires: dbus-devel BuildRequires: dbus-libs %if 0%{?rhel5_minor} >= 7 diff --git a/src/external/samba.m4 b/src/external/samba.m4 index 735cc5a187dba7514a9c7436a6d3ba948ad522b0..bf790365468da5ccdd40a260d74893490b7e7be8 100644 --- a/src/external/samba.m4 +++ b/src/external/samba.m4 @@ -19,4 +19,12 @@ If you do not want to build these providers it is possible to build SSSD without them. In this case, you will need to execute configure script with argument --without-samba ]]))
- PKG_CHECK_MODULES(INI_CONFIG, ini_config >= 1.0.0, ,
^^^^^ 1.1.0 You used correct version in spec file.
configure will pass with old version of ding-libs and compilation fail in gpo_child.
AC_MSG_ERROR([[Please install libini_config development libraries.
+libini_config libraries are necessary for building ad and ipa provider.
^^^^^^^^^ You touch this code. I might be good to slightly modify text. "ad with gpo" or better wording.
Sorry, for nitpicking. I would ignore it there was not problem with version.
+If you do not want to build these providers it is possible to build SSSD +without them. In this case, you will need to execute configure script +with argument --without-samba
- ]]))
fi
//snip
+/* == ad_gpo_process_cse_send/recv helpers ================================= */
+static errno_t +create_cse_send_buffer(TALLOC_CTX *mem_ctx,
char *smb_uri,
struct io_buffer **io_buf)
+{
- struct io_buffer *buf;
- size_t rp;
- int smb_uri_length;
- smb_uri_length = strlen(smb_uri);
- DEBUG(SSSDBG_TRACE_FUNC, "smb_uri: %s\n", smb_uri);
- DEBUG(SSSDBG_TRACE_FUNC, "strlen(smb_uri): %d\n", smb_uri_length);
- buf = talloc(mem_ctx, struct io_buffer);
- if (buf == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "talloc failed.\n");
return ENOMEM;
- }
- buf->size = 1 * sizeof(uint32_t);
- if (smb_uri) {
I thing that this test is redundant, because strlen would crash anyway with NULL.
Error: REVERSE_INULL sssd-1.11.92/src/providers/ad/ad_gpo.c:2792: deref_ptr_in_call: Dereferencing pointer "smb_uri". sssd-1.11.92/src/providers/ad/ad_gpo.c:2804: check_after_deref: Null-checking "smb_uri" suggests that it may be null, but it has already been dereferenced on all paths leading to the check.
buf->size += smb_uri_length;
- }
- DEBUG(SSSDBG_TRACE_ALL, "buffer size: %zu\n", buf->size);
- buf->data = talloc_size(buf, buf->size);
- if (buf->data == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "talloc_size failed.\n");
talloc_free(buf);
return ENOMEM;
- }
- rp = 0;
- /* smb_uri */
- if (smb_uri) {
SAFEALIGN_SET_UINT32(&buf->data[rp], smb_uri_length, &rp);
safealign_memcpy(&buf->data[rp], smb_uri, smb_uri_length, &rp);
- } else {
SAFEALIGN_SET_UINT32(&buf->data[rp], 0, &rp);
- }
- *io_buf = buf;
- return EOK;
+}
I am attaching another html report from clang.
I let final ACK for Sumit.
LS
Revised patches attached.
Regards, Yassir.
ACK
bye, Sumit
On Mon, Jun 30, 2014 at 10:17:48AM +0200, Sumit Bose wrote:
On Sat, Jun 28, 2014 at 01:30:36AM -0400, Yassir Elley wrote:
----- Original Message -----
On (25/06/14 23:33), Yassir Elley wrote:
I have attached a revised first patch which addresses the code review comments. I'm not sure if I needed to (since it has already been ACK'ed), but I have also attached an unchanged second patch.
Thanks, Yassir.
I know that Sumit already ACKed these patches, but I used them to test CI script and I found some minor issues
- warnings from static analysers (as usual :-)
From b58f39645872bc1dd878ac650861af89964101f9 Mon Sep 17 00:00:00 2001 From: Yassir Elley yelley@redhat.com Date: Wed, 21 May 2014 15:11:36 -0400 Subject: [PATCH 1/2] AD-GPO: Add gpo-smb implementation in gpo_child process
Makefile.am | 22 + contrib/sssd.spec.in | 2 +- src/external/samba.m4 | 8 + src/providers/ad/ad_gpo.c | 887 +++++++++++++++++++++++++++++++++++++++- src/providers/ad/ad_gpo_child.c | 637 +++++++++++++++++++++++++++++ 5 files changed, 1543 insertions(+), 13 deletions(-) create mode 100644 src/providers/ad/ad_gpo_child.c
diff --git a/Makefile.am b/Makefile.am index 83999f3584fe130320ebfa6ea459f5706992c6a4..6cf5d8cb0c7c68314dad49461e7cd1fb4452bc1b 100644 --- a/Makefile.am +++ b/Makefile.am @@ -121,6 +121,9 @@ endif if BUILD_IFP sssdlibexec_PROGRAMS += sssd_ifp endif +if BUILD_SAMBA +sssdlibexec_PROGRAMS += gpo_child +endif
if BUILD_PAC_RESPONDER @@ -2298,6 +2301,25 @@ ldap_child_LDADD = \ $(DHASH_LIBS) \ $(KRB5_LIBS)
+gpo_child_SOURCES = \
- src/providers/ad/ad_gpo_child.c \
- src/util/atomic_io.c \
- src/util/util.c \
- src/util/signal.c
+gpo_child_CFLAGS = \
- $(AM_CFLAGS) \
- $(POPT_CFLAGS) \
- $(KRB5_CFLAGS) \
- $(INI_CONFIG_CFLAGS) \
- $(SMBCLIENT_CFLAGS)
+gpo_child_LDADD = \
- libsss_debug.la \
- $(TALLOC_LIBS) \
- $(POPT_LIBS) \
- $(DHASH_LIBS) \
- $(INI_CONFIG_LIBS) \
- $(SMBCLIENT_LIBS)
proxy_child_SOURCES = \ src/providers/proxy/proxy_child.c \ src/providers/data_provider_iface_generated.c \ diff --git a/contrib/sssd.spec.in b/contrib/sssd.spec.in index b5e7903713ccc6944fbed4f84c2f0413a257b760..10704ae48cf5636de2a5cbbb42fc00b99052144f 100644 --- a/contrib/sssd.spec.in +++ b/contrib/sssd.spec.in @@ -97,7 +97,7 @@ BuildRequires: libtdb-devel BuildRequires: libldb-devel BuildRequires: libdhash-devel >= 0.4.2 BuildRequires: libcollection-devel -BuildRequires: libini_config-devel +BuildRequires: libini_config-devel >= 1.1 BuildRequires: dbus-devel BuildRequires: dbus-libs %if 0%{?rhel5_minor} >= 7 diff --git a/src/external/samba.m4 b/src/external/samba.m4 index 735cc5a187dba7514a9c7436a6d3ba948ad522b0..bf790365468da5ccdd40a260d74893490b7e7be8 100644 --- a/src/external/samba.m4 +++ b/src/external/samba.m4 @@ -19,4 +19,12 @@ If you do not want to build these providers it is possible to build SSSD without them. In this case, you will need to execute configure script with argument --without-samba ]]))
- PKG_CHECK_MODULES(INI_CONFIG, ini_config >= 1.0.0, ,
^^^^^ 1.1.0 You used correct version in spec file.
configure will pass with old version of ding-libs and compilation fail in gpo_child.
AC_MSG_ERROR([[Please install libini_config development libraries.
+libini_config libraries are necessary for building ad and ipa provider.
^^^^^^^^^ You touch this code. I might be good to slightly modify text. "ad with gpo" or better wording.
Sorry, for nitpicking. I would ignore it there was not problem with version.
+If you do not want to build these providers it is possible to build SSSD +without them. In this case, you will need to execute configure script +with argument --without-samba
- ]]))
fi
//snip
+/* == ad_gpo_process_cse_send/recv helpers ================================= */
+static errno_t +create_cse_send_buffer(TALLOC_CTX *mem_ctx,
char *smb_uri,
struct io_buffer **io_buf)
+{
- struct io_buffer *buf;
- size_t rp;
- int smb_uri_length;
- smb_uri_length = strlen(smb_uri);
- DEBUG(SSSDBG_TRACE_FUNC, "smb_uri: %s\n", smb_uri);
- DEBUG(SSSDBG_TRACE_FUNC, "strlen(smb_uri): %d\n", smb_uri_length);
- buf = talloc(mem_ctx, struct io_buffer);
- if (buf == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "talloc failed.\n");
return ENOMEM;
- }
- buf->size = 1 * sizeof(uint32_t);
- if (smb_uri) {
I thing that this test is redundant, because strlen would crash anyway with NULL.
Error: REVERSE_INULL sssd-1.11.92/src/providers/ad/ad_gpo.c:2792: deref_ptr_in_call: Dereferencing pointer "smb_uri". sssd-1.11.92/src/providers/ad/ad_gpo.c:2804: check_after_deref: Null-checking "smb_uri" suggests that it may be null, but it has already been dereferenced on all paths leading to the check.
buf->size += smb_uri_length;
- }
- DEBUG(SSSDBG_TRACE_ALL, "buffer size: %zu\n", buf->size);
- buf->data = talloc_size(buf, buf->size);
- if (buf->data == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "talloc_size failed.\n");
talloc_free(buf);
return ENOMEM;
- }
- rp = 0;
- /* smb_uri */
- if (smb_uri) {
SAFEALIGN_SET_UINT32(&buf->data[rp], smb_uri_length, &rp);
safealign_memcpy(&buf->data[rp], smb_uri, smb_uri_length, &rp);
- } else {
SAFEALIGN_SET_UINT32(&buf->data[rp], 0, &rp);
- }
- *io_buf = buf;
- return EOK;
+}
I am attaching another html report from clang.
I let final ACK for Sumit.
LS
Revised patches attached.
Regards, Yassir.
ACK
bye, Sumit
Great work, thank you!
Pushed to master: 19d3aba12c70528708be9440aca66038a291f29e d3ca320a1ddea52fe86c052dd5521b8f98bb4f9f
On Sat, Jun 14, 2014 at 09:01:53PM -0400, Yassir Elley wrote:
----- Original Message -----
On Thu, Jun 12, 2014 at 04:37:25PM -0400, Yassir Elley wrote:
...
Second, you are only checking SeInteractiveLogonRight and SeDenyInteractiveLogonRight. What about Se[Deny][Network|Batch|Service|RemoteInteractive]LogonRight? Are you planning to integrate them as well and relate them to different PAM services?
The plan for the initial deployment is to only support SeInteractiveLogonRight and SeDenyInteractiveLogonRight.
ok, but please keep in mind that it would be hard to add support for different login types, e.g. network vs. console, later on without breaking backward compatibility.
We are initially supporting console login as a feature. If we later decide to support network login as a feature (in a subsequent release), what is the concern regarding backward compatibility?
currently the patches add access control to all services not only console login. This means that currently SeInteractiveLogonRight SeDenyInteractiveLogonRight catches all services. If later on you want to add support for Se[Deny]NetworkLogonRight you either have to restrict the scope of Sei[Deny]InteractiveLogonRight which is not backward compatible. Or admins have to explicitly list all users and groups which should only be allowed to log in over the network in SeDenyInteractiveLogonRight which afaik is not needed for Windows clients.
bye, Sumit
----- Original Message -----
On Sat, Jun 14, 2014 at 09:01:53PM -0400, Yassir Elley wrote:
----- Original Message -----
On Thu, Jun 12, 2014 at 04:37:25PM -0400, Yassir Elley wrote:
...
Second, you are only checking SeInteractiveLogonRight and SeDenyInteractiveLogonRight. What about Se[Deny][Network|Batch|Service|RemoteInteractive]LogonRight? Are you planning to integrate them as well and relate them to different PAM services?
The plan for the initial deployment is to only support SeInteractiveLogonRight and SeDenyInteractiveLogonRight.
ok, but please keep in mind that it would be hard to add support for different login types, e.g. network vs. console, later on without breaking backward compatibility.
We are initially supporting console login as a feature. If we later decide to support network login as a feature (in a subsequent release), what is the concern regarding backward compatibility?
currently the patches add access control to all services not only console login. This means that currently SeInteractiveLogonRight SeDenyInteractiveLogonRight catches all services. If later on you want to add support for Se[Deny]NetworkLogonRight you either have to restrict the scope of Sei[Deny]InteractiveLogonRight which is not backward compatible. Or admins have to explicitly list all users and groups which should only be allowed to log in over the network in SeDenyInteractiveLogonRight which afaik is not needed for Windows clients.
bye, Sumit
Of the various logon rights supported by GPO, I think the InteractiveLogonRight and NetworkLogonRight map well to the Linux world ("local" logon and "network" logon), while the other logon rights (Batch, Service, RemoteInteractive) are more Windows-specific. So I think we should focus on these two use cases ("local" vs "network").
You make a good point regarding backwards-compatibility. If we decide to later add support for gpo processing of network logons, this could result in users that were previously allowed logon access (using the presumably more liberal local logon rules) to be denied access (using the presumably more restrictive network logon rules). So I think it would be a good idea to support both InteractiveLogonRight (aka local/console) and NetworkLogonRight from the very beginning (and their Deny counterparts) to avoid such backward-compatibility issues.
Having said that, this will introduce some complexity since it will require us to specify which pam service names are considered "local" by sssd (and should use InteractiveLogonRight policy) and which are considered "network" (and should use NetworkLogonRight policy). We would also need to take into account that pam allows custom service names to be used. A straightforward approach would be to specify (in man page?) the pam services we consider to be "local" (e.g. login, gdm, su) and then state that all other services (including custom services, as well as services that wouldn't be expected to make pam_acct_mgmt calls) are considered "network". A more sophisticated approach would additionally provide an api that allows applications to specify whether a given service name should be considered "local" or "network". What do you think?
Regards, Yassir.
sssd-devel@lists.fedorahosted.org