----- 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(a)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(a)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(a)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).
> + * 1) The "allowed_sids_condition" is satisfied if any of the
> principal_sids
> + * appears in allowed_sids OR if the allowed_sids list is empty
> + * 2) 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.