[SSSD] Re: [PATCH] Abstract and improve connection credential handling
On (27/01/16 17:13), Michal Židek wrote:
>On 01/27/2016 01:03 PM, Lukas Slebodnik wrote:
>>On (26/01/16 18:08), Michal Židek wrote:
>>>On 01/26/2016 02:54 PM, Lukas Slebodnik wrote:
On (26/01/16 14:51), Michal Židek wrote:
>On 01/21/2016 03:51 PM, Simo Sorce wrote:
>>On Wed, 2016-01-20 at 16:38 +0100, Lukas Slebodnik wrote:
>>>On (19/01/16 15:38), Simo Sorce wrote:
On Tue, 2016-01-19 at 10:34 -0500, Simo Sorce wrote:
>On Tue, 2016-01-19 at 11:23 +0100, Lukas Slebodnik wrote:
>[...]
>>>+#endif /* __SSSD_UTIL_SELINUX_H__ */
>>BTW will we need this header file if we make
>>struct cli_creds opaque?
>
>Replying to both your mails here:
>Making cli_creds opaque requires using a pointer and dealing with
>allocating it, I guess I can do that, the headers file would still be
>needed in order to avoid huge ifdefs around the functions that
>implement
>handling SELinux stuff. It makes the code a lot more readable and
>searchable.
>
>Simo.
>
Attached find patch that changes the code so that cli_creds is now
opaque.
This *should* work w/o the patch that changes the headers but I haven't
tested it w/o that patch yet.
>>>I had an issue to build it correctly with ifp responder.
>>>src/responder/ifp/ifpsrv_util.c: In function ‘ifp_req_create’:
>>>src/responder/ifp/ifpsrv_util.c:64:30: error: passing argument 1 of
>>>‘check_allowed_uids’ makes pointer from integer without a cast
>>>[-Werror=int-conversion]
>>> ret = check_allowed_uids(dbus_req->client,
>>> ^
>>>In file included from ../sssd/src/responder/ifp/ifp_private.h:27:0,
>>> from ../sssd/src/responder/ifp/ifpsrv_util.c:27:
>>>src/responder/common/responder.h:334:9: note: expected ‘struct cli_creds
>>>*’ but argument is of type ‘int64_t {aka long int}’
>>> errno_t check_allowed_uids(struct cli_creds *creds,
>>> ^
>>>
>>>ifp responder uses different way to obtain uid. I changed back the
>>>prototype of
>>>check_allowed_uids.
>>>Here is a diff on to of your patch.
>>>
>>>diff --git a/src/responder/common/responder.h
>>>b/src/responder/common/responder.h
>>>index 419a863..3b70b69 100644
>>>--- a/src/responder/common/responder.h
>>>+++ b/src/responder/common/responder.h
>>>@@ -331,8 +331,7 @@ errno_t csv_string_to_uid_array(TALLOC_CTX *mem_ctx,
>>>const char *csv_string,
>>> size_t *_uid_count, uid_t **_uids);
>>>
>>> uid_t client_euid(struct cli_creds *creds);
>>>-errno_t check_allowed_uids(struct cli_creds *creds,
>>>- size_t allowed_uids_count,
>>>+errno_t check_allowed_uids(uid_t uid, size_t allowed_uids_count,
>>> uid_t *allowed_uids);
>>>
>>> struct tevent_req *
>>>diff --git a/src/responder/common/responder_common.c
>>>b/src/responder/common/responder_common.c
>>>index 27193ce..6ac1ea2 100644
>>>--- a/src/responder/common/responder_common.c
>>>+++ b/src/responder/common/responder_common.c
>>>@@ -138,8 +138,7 @@ uid_t client_euid(struct cli_creds *creds)
>>> return cli_creds_get_uid(creds);
>>> }
>>>
>>>-errno_t check_allowed_uids(struct cli_creds *creds,
>>>- size_t allowed_uids_count,
>>>+errno_t check_allowed_uids(uid_t uid, size_t allowed_uids_count,
>>> uid_t *allowed_uids)
>>> {
>>> size_t c;
>>>@@ -149,7 +148,7 @@ errno_t check_allowed_uids(struct cli_creds *creds,
>>> }
>>>
>>> for (c = 0; c < allowed_uids_count; c++) {
>>>-if (client_euid(creds) == allowed_uids[c]) {
>>>+if (uid == allowed_uids[c]) {
>>> return EOK;
>>> }
>>> }
>>>@@ -449,12 +448,13 @@ static void accept_fd_handler(struct
>>>tevent_context *ev,
>>> return;
>>> }
>>>
>>>-ret = check_allowed_uids(cctx->creds, rctx->allowed_uids_count,
>>>+ret = check_allowed_uids(client_euid(cctx->creds),
>>>rctx->allowed_uids_count,
>>> rctx->allowed_uids);
>>> if (ret != EOK) {
>>> if (ret == EACCES) {
>>>-DEBUG(SSSDBG_CRIT_FAILURE, "Access denied for uid
>>>[%d].\n",
>>>-
>>>(int)client_euid(cctx->creds));
>>>+DEBUG(SSSDBG_CRIT_FAILURE,
>>>+ "Access denied for uid [%"SPRIuid"].\n",
>>>+ client_euid(cctx->creds));
>>> } else {
>>> DEBUG(SSSDBG_OP_FAILURE, "check_allowed_uids
>>> fai
[SSSD] Re: [PATCH] Abstract and improve connection credential handling
On 01/27/2016 01:03 PM, Lukas Slebodnik wrote:
On (26/01/16 18:08), Michal Židek wrote:
On 01/26/2016 02:54 PM, Lukas Slebodnik wrote:
On (26/01/16 14:51), Michal Židek wrote:
On 01/21/2016 03:51 PM, Simo Sorce wrote:
On Wed, 2016-01-20 at 16:38 +0100, Lukas Slebodnik wrote:
On (19/01/16 15:38), Simo Sorce wrote:
On Tue, 2016-01-19 at 10:34 -0500, Simo Sorce wrote:
On Tue, 2016-01-19 at 11:23 +0100, Lukas Slebodnik wrote:
[...]
+#endif /* __SSSD_UTIL_SELINUX_H__ */
BTW will we need this header file if we make
struct cli_creds opaque?
Replying to both your mails here:
Making cli_creds opaque requires using a pointer and dealing with
allocating it, I guess I can do that, the headers file would still be
needed in order to avoid huge ifdefs around the functions that implement
handling SELinux stuff. It makes the code a lot more readable and
searchable.
Simo.
Attached find patch that changes the code so that cli_creds is now
opaque.
This *should* work w/o the patch that changes the headers but I haven't
tested it w/o that patch yet.
I had an issue to build it correctly with ifp responder.
src/responder/ifp/ifpsrv_util.c: In function ‘ifp_req_create’:
src/responder/ifp/ifpsrv_util.c:64:30: error: passing argument 1 of
‘check_allowed_uids’ makes pointer from integer without a cast
[-Werror=int-conversion]
ret = check_allowed_uids(dbus_req->client,
^
In file included from ../sssd/src/responder/ifp/ifp_private.h:27:0,
from ../sssd/src/responder/ifp/ifpsrv_util.c:27:
src/responder/common/responder.h:334:9: note: expected ‘struct cli_creds *’ but
argument is of type ‘int64_t {aka long int}’
errno_t check_allowed_uids(struct cli_creds *creds,
^
ifp responder uses different way to obtain uid. I changed back the prototype of
check_allowed_uids.
Here is a diff on to of your patch.
diff --git a/src/responder/common/responder.h b/src/responder/common/responder.h
index 419a863..3b70b69 100644
--- a/src/responder/common/responder.h
+++ b/src/responder/common/responder.h
@@ -331,8 +331,7 @@ errno_t csv_string_to_uid_array(TALLOC_CTX *mem_ctx, const
char *csv_string,
size_t *_uid_count, uid_t **_uids);
uid_t client_euid(struct cli_creds *creds);
-errno_t check_allowed_uids(struct cli_creds *creds,
- size_t allowed_uids_count,
+errno_t check_allowed_uids(uid_t uid, size_t allowed_uids_count,
uid_t *allowed_uids);
struct tevent_req *
diff --git a/src/responder/common/responder_common.c
b/src/responder/common/responder_common.c
index 27193ce..6ac1ea2 100644
--- a/src/responder/common/responder_common.c
+++ b/src/responder/common/responder_common.c
@@ -138,8 +138,7 @@ uid_t client_euid(struct cli_creds *creds)
return cli_creds_get_uid(creds);
}
-errno_t check_allowed_uids(struct cli_creds *creds,
- size_t allowed_uids_count,
+errno_t check_allowed_uids(uid_t uid, size_t allowed_uids_count,
uid_t *allowed_uids)
{
size_t c;
@@ -149,7 +148,7 @@ errno_t check_allowed_uids(struct cli_creds *creds,
}
for (c = 0; c < allowed_uids_count; c++) {
-if (client_euid(creds) == allowed_uids[c]) {
+if (uid == allowed_uids[c]) {
return EOK;
}
}
@@ -449,12 +448,13 @@ static void accept_fd_handler(struct tevent_context *ev,
return;
}
-ret = check_allowed_uids(cctx->creds, rctx->allowed_uids_count,
+ret = check_allowed_uids(client_euid(cctx->creds),
rctx->allowed_uids_count,
rctx->allowed_uids);
if (ret != EOK) {
if (ret == EACCES) {
-DEBUG(SSSDBG_CRIT_FAILURE, "Access denied for uid [%d].\n",
-(int)client_euid(cctx->creds));
+DEBUG(SSSDBG_CRIT_FAILURE,
+ "Access denied for uid [%"SPRIuid"].\n",
+ client_euid(cctx->creds));
} else {
DEBUG(SSSDBG_OP_FAILURE, "check_allowed_uids failed.\n");
}
diff --git a/src/responder/pam/pamsrv_cmd.c b/src/responder/pam/pamsrv_cmd.c
index 5fe3e6b..bfc534f 100644
--- a/src/responder/pam/pamsrv_cmd.c
+++ b/src/responder/pam/pamsrv_cmd.c
@@ -1001,7 +1001,7 @@ static bool is_uid_trusted(struct cli_creds *creds,
return true;
}
-ret = check_allowed_uids(creds, trusted_uids_count, trusted_uids);
+ret = check_allowed_uids(client_euid(creds), trusted_uids_count,
trusted_uids);
if (ret == EOK) return true;
return false;
@@ -1091,13 +1091,13 @@ static int pam_forwarder(struct cli_ctx *cctx, int
pam_cmd)
}
pd = preq->pd;
-preq->is_uid_trusted = is_uid_trusted(&cctx->creds,
+preq->is_uid_trusted = is_uid_trusted(cctx->creds,
pctx->trusted_uids_count,
[SSSD] Re: [PATCH] Abstract and improve connection credential handling
On (26/01/16 18:08), Michal Židek wrote:
>On 01/26/2016 02:54 PM, Lukas Slebodnik wrote:
>>On (26/01/16 14:51), Michal Židek wrote:
>>>On 01/21/2016 03:51 PM, Simo Sorce wrote:
On Wed, 2016-01-20 at 16:38 +0100, Lukas Slebodnik wrote:
>On (19/01/16 15:38), Simo Sorce wrote:
>>On Tue, 2016-01-19 at 10:34 -0500, Simo Sorce wrote:
>>>On Tue, 2016-01-19 at 11:23 +0100, Lukas Slebodnik wrote:
>>>[...]
>+#endif /* __SSSD_UTIL_SELINUX_H__ */
BTW will we need this header file if we make
struct cli_creds opaque?
>>>
>>>Replying to both your mails here:
>>>Making cli_creds opaque requires using a pointer and dealing with
>>>allocating it, I guess I can do that, the headers file would still be
>>>needed in order to avoid huge ifdefs around the functions that implement
>>>handling SELinux stuff. It makes the code a lot more readable and
>>>searchable.
>>>
>>>Simo.
>>>
>>
>>Attached find patch that changes the code so that cli_creds is now
>>opaque.
>>This *should* work w/o the patch that changes the headers but I haven't
>>tested it w/o that patch yet.
>>
>I had an issue to build it correctly with ifp responder.
>src/responder/ifp/ifpsrv_util.c: In function ‘ifp_req_create’:
>src/responder/ifp/ifpsrv_util.c:64:30: error: passing argument 1 of
>‘check_allowed_uids’ makes pointer from integer without a cast
>[-Werror=int-conversion]
> ret = check_allowed_uids(dbus_req->client,
> ^
>In file included from ../sssd/src/responder/ifp/ifp_private.h:27:0,
> from ../sssd/src/responder/ifp/ifpsrv_util.c:27:
>src/responder/common/responder.h:334:9: note: expected ‘struct cli_creds
>*’ but argument is of type ‘int64_t {aka long int}’
> errno_t check_allowed_uids(struct cli_creds *creds,
> ^
>
>ifp responder uses different way to obtain uid. I changed back the
>prototype of
>check_allowed_uids.
>Here is a diff on to of your patch.
>
>diff --git a/src/responder/common/responder.h
>b/src/responder/common/responder.h
>index 419a863..3b70b69 100644
>--- a/src/responder/common/responder.h
>+++ b/src/responder/common/responder.h
>@@ -331,8 +331,7 @@ errno_t csv_string_to_uid_array(TALLOC_CTX *mem_ctx,
>const char *csv_string,
> size_t *_uid_count, uid_t **_uids);
>
> uid_t client_euid(struct cli_creds *creds);
>-errno_t check_allowed_uids(struct cli_creds *creds,
>- size_t allowed_uids_count,
>+errno_t check_allowed_uids(uid_t uid, size_t allowed_uids_count,
> uid_t *allowed_uids);
>
> struct tevent_req *
>diff --git a/src/responder/common/responder_common.c
>b/src/responder/common/responder_common.c
>index 27193ce..6ac1ea2 100644
>--- a/src/responder/common/responder_common.c
>+++ b/src/responder/common/responder_common.c
>@@ -138,8 +138,7 @@ uid_t client_euid(struct cli_creds *creds)
> return cli_creds_get_uid(creds);
> }
>
>-errno_t check_allowed_uids(struct cli_creds *creds,
>- size_t allowed_uids_count,
>+errno_t check_allowed_uids(uid_t uid, size_t allowed_uids_count,
> uid_t *allowed_uids)
> {
> size_t c;
>@@ -149,7 +148,7 @@ errno_t check_allowed_uids(struct cli_creds *creds,
> }
>
> for (c = 0; c < allowed_uids_count; c++) {
>-if (client_euid(creds) == allowed_uids[c]) {
>+if (uid == allowed_uids[c]) {
> return EOK;
> }
> }
>@@ -449,12 +448,13 @@ static void accept_fd_handler(struct tevent_context
>*ev,
> return;
> }
>
>-ret = check_allowed_uids(cctx->creds, rctx->allowed_uids_count,
>+ret = check_allowed_uids(client_euid(cctx->creds),
>rctx->allowed_uids_count,
> rctx->allowed_uids);
> if (ret != EOK) {
> if (ret == EACCES) {
>-DEBUG(SSSDBG_CRIT_FAILURE, "Access denied for uid
>[%d].\n",
>-
>(int)client_euid(cctx->creds));
>+DEBUG(SSSDBG_CRIT_FAILURE,
>+ "Access denied for uid [%"SPRIuid"].\n",
>+ client_euid(cctx->creds));
> } else {
> DEBUG(SSSDBG_OP_FAILURE, "check_allowed_uids failed.\n");
> }
>diff --git a/src/responder/pam/pamsrv_cmd.c
>b/src/responder/pam/pamsrv_cmd.c
>index 5fe3e6b..bfc534f 100644
>--- a/src/responder/pam/pamsrv_cmd.c
>+++ b/src/responder/pam/pamsrv_cmd.c
>@@ -1001,7 +1001,7 @@ static bool is_uid_trusted(struct cli_creds *creds,
>
[SSSD] Re: [PATCH] Abstract and improve connection credential handling
On 01/26/2016 02:54 PM, Lukas Slebodnik wrote:
On (26/01/16 14:51), Michal Židek wrote:
On 01/21/2016 03:51 PM, Simo Sorce wrote:
On Wed, 2016-01-20 at 16:38 +0100, Lukas Slebodnik wrote:
On (19/01/16 15:38), Simo Sorce wrote:
On Tue, 2016-01-19 at 10:34 -0500, Simo Sorce wrote:
On Tue, 2016-01-19 at 11:23 +0100, Lukas Slebodnik wrote:
[...]
+#endif /* __SSSD_UTIL_SELINUX_H__ */
BTW will we need this header file if we make
struct cli_creds opaque?
Replying to both your mails here:
Making cli_creds opaque requires using a pointer and dealing with
allocating it, I guess I can do that, the headers file would still be
needed in order to avoid huge ifdefs around the functions that implement
handling SELinux stuff. It makes the code a lot more readable and
searchable.
Simo.
Attached find patch that changes the code so that cli_creds is now
opaque.
This *should* work w/o the patch that changes the headers but I haven't
tested it w/o that patch yet.
I had an issue to build it correctly with ifp responder.
src/responder/ifp/ifpsrv_util.c: In function ‘ifp_req_create’:
src/responder/ifp/ifpsrv_util.c:64:30: error: passing argument 1 of
‘check_allowed_uids’ makes pointer from integer without a cast
[-Werror=int-conversion]
ret = check_allowed_uids(dbus_req->client,
^
In file included from ../sssd/src/responder/ifp/ifp_private.h:27:0,
from ../sssd/src/responder/ifp/ifpsrv_util.c:27:
src/responder/common/responder.h:334:9: note: expected ‘struct cli_creds *’ but
argument is of type ‘int64_t {aka long int}’
errno_t check_allowed_uids(struct cli_creds *creds,
^
ifp responder uses different way to obtain uid. I changed back the prototype of
check_allowed_uids.
Here is a diff on to of your patch.
diff --git a/src/responder/common/responder.h b/src/responder/common/responder.h
index 419a863..3b70b69 100644
--- a/src/responder/common/responder.h
+++ b/src/responder/common/responder.h
@@ -331,8 +331,7 @@ errno_t csv_string_to_uid_array(TALLOC_CTX *mem_ctx, const
char *csv_string,
size_t *_uid_count, uid_t **_uids);
uid_t client_euid(struct cli_creds *creds);
-errno_t check_allowed_uids(struct cli_creds *creds,
- size_t allowed_uids_count,
+errno_t check_allowed_uids(uid_t uid, size_t allowed_uids_count,
uid_t *allowed_uids);
struct tevent_req *
diff --git a/src/responder/common/responder_common.c
b/src/responder/common/responder_common.c
index 27193ce..6ac1ea2 100644
--- a/src/responder/common/responder_common.c
+++ b/src/responder/common/responder_common.c
@@ -138,8 +138,7 @@ uid_t client_euid(struct cli_creds *creds)
return cli_creds_get_uid(creds);
}
-errno_t check_allowed_uids(struct cli_creds *creds,
- size_t allowed_uids_count,
+errno_t check_allowed_uids(uid_t uid, size_t allowed_uids_count,
uid_t *allowed_uids)
{
size_t c;
@@ -149,7 +148,7 @@ errno_t check_allowed_uids(struct cli_creds *creds,
}
for (c = 0; c < allowed_uids_count; c++) {
-if (client_euid(creds) == allowed_uids[c]) {
+if (uid == allowed_uids[c]) {
return EOK;
}
}
@@ -449,12 +448,13 @@ static void accept_fd_handler(struct tevent_context *ev,
return;
}
-ret = check_allowed_uids(cctx->creds, rctx->allowed_uids_count,
+ret = check_allowed_uids(client_euid(cctx->creds),
rctx->allowed_uids_count,
rctx->allowed_uids);
if (ret != EOK) {
if (ret == EACCES) {
-DEBUG(SSSDBG_CRIT_FAILURE, "Access denied for uid [%d].\n",
-(int)client_euid(cctx->creds));
+DEBUG(SSSDBG_CRIT_FAILURE,
+ "Access denied for uid [%"SPRIuid"].\n",
+ client_euid(cctx->creds));
} else {
DEBUG(SSSDBG_OP_FAILURE, "check_allowed_uids failed.\n");
}
diff --git a/src/responder/pam/pamsrv_cmd.c b/src/responder/pam/pamsrv_cmd.c
index 5fe3e6b..bfc534f 100644
--- a/src/responder/pam/pamsrv_cmd.c
+++ b/src/responder/pam/pamsrv_cmd.c
@@ -1001,7 +1001,7 @@ static bool is_uid_trusted(struct cli_creds *creds,
return true;
}
-ret = check_allowed_uids(creds, trusted_uids_count, trusted_uids);
+ret = check_allowed_uids(client_euid(creds), trusted_uids_count,
trusted_uids);
if (ret == EOK) return true;
return false;
@@ -1091,13 +1091,13 @@ static int pam_forwarder(struct cli_ctx *cctx, int
pam_cmd)
}
pd = preq->pd;
-preq->is_uid_trusted = is_uid_trusted(&cctx->creds,
+preq->is_uid_trusted = is_uid_trusted(cctx->creds,
pctx->trusted_uids_count,
pctx->trusted_uids);
if (!preq->is_uid_tr
[SSSD] Re: [PATCH] Abstract and improve connection credential handling
On (26/01/16 14:51), Michal Židek wrote:
>On 01/21/2016 03:51 PM, Simo Sorce wrote:
>>On Wed, 2016-01-20 at 16:38 +0100, Lukas Slebodnik wrote:
>>>On (19/01/16 15:38), Simo Sorce wrote:
On Tue, 2016-01-19 at 10:34 -0500, Simo Sorce wrote:
>On Tue, 2016-01-19 at 11:23 +0100, Lukas Slebodnik wrote:
>[...]
>>>+#endif /* __SSSD_UTIL_SELINUX_H__ */
>>BTW will we need this header file if we make
>>struct cli_creds opaque?
>
>Replying to both your mails here:
>Making cli_creds opaque requires using a pointer and dealing with
>allocating it, I guess I can do that, the headers file would still be
>needed in order to avoid huge ifdefs around the functions that implement
>handling SELinux stuff. It makes the code a lot more readable and
>searchable.
>
>Simo.
>
Attached find patch that changes the code so that cli_creds is now
opaque.
This *should* work w/o the patch that changes the headers but I haven't
tested it w/o that patch yet.
>>>I had an issue to build it correctly with ifp responder.
>>>src/responder/ifp/ifpsrv_util.c: In function ‘ifp_req_create’:
>>>src/responder/ifp/ifpsrv_util.c:64:30: error: passing argument 1 of
>>>‘check_allowed_uids’ makes pointer from integer without a cast
>>>[-Werror=int-conversion]
>>> ret = check_allowed_uids(dbus_req->client,
>>> ^
>>>In file included from ../sssd/src/responder/ifp/ifp_private.h:27:0,
>>> from ../sssd/src/responder/ifp/ifpsrv_util.c:27:
>>>src/responder/common/responder.h:334:9: note: expected ‘struct cli_creds *’
>>>but argument is of type ‘int64_t {aka long int}’
>>> errno_t check_allowed_uids(struct cli_creds *creds,
>>> ^
>>>
>>>ifp responder uses different way to obtain uid. I changed back the prototype
>>>of
>>>check_allowed_uids.
>>>Here is a diff on to of your patch.
>>>
>>>diff --git a/src/responder/common/responder.h
>>>b/src/responder/common/responder.h
>>>index 419a863..3b70b69 100644
>>>--- a/src/responder/common/responder.h
>>>+++ b/src/responder/common/responder.h
>>>@@ -331,8 +331,7 @@ errno_t csv_string_to_uid_array(TALLOC_CTX *mem_ctx,
>>>const char *csv_string,
>>> size_t *_uid_count, uid_t **_uids);
>>>
>>> uid_t client_euid(struct cli_creds *creds);
>>>-errno_t check_allowed_uids(struct cli_creds *creds,
>>>- size_t allowed_uids_count,
>>>+errno_t check_allowed_uids(uid_t uid, size_t allowed_uids_count,
>>> uid_t *allowed_uids);
>>>
>>> struct tevent_req *
>>>diff --git a/src/responder/common/responder_common.c
>>>b/src/responder/common/responder_common.c
>>>index 27193ce..6ac1ea2 100644
>>>--- a/src/responder/common/responder_common.c
>>>+++ b/src/responder/common/responder_common.c
>>>@@ -138,8 +138,7 @@ uid_t client_euid(struct cli_creds *creds)
>>> return cli_creds_get_uid(creds);
>>> }
>>>
>>>-errno_t check_allowed_uids(struct cli_creds *creds,
>>>- size_t allowed_uids_count,
>>>+errno_t check_allowed_uids(uid_t uid, size_t allowed_uids_count,
>>> uid_t *allowed_uids)
>>> {
>>> size_t c;
>>>@@ -149,7 +148,7 @@ errno_t check_allowed_uids(struct cli_creds *creds,
>>> }
>>>
>>> for (c = 0; c < allowed_uids_count; c++) {
>>>-if (client_euid(creds) == allowed_uids[c]) {
>>>+if (uid == allowed_uids[c]) {
>>> return EOK;
>>> }
>>> }
>>>@@ -449,12 +448,13 @@ static void accept_fd_handler(struct tevent_context
>>>*ev,
>>> return;
>>> }
>>>
>>>-ret = check_allowed_uids(cctx->creds, rctx->allowed_uids_count,
>>>+ret = check_allowed_uids(client_euid(cctx->creds),
>>>rctx->allowed_uids_count,
>>> rctx->allowed_uids);
>>> if (ret != EOK) {
>>> if (ret == EACCES) {
>>>-DEBUG(SSSDBG_CRIT_FAILURE, "Access denied for uid [%d].\n",
>>>-(int)client_euid(cctx->creds));
>>>+DEBUG(SSSDBG_CRIT_FAILURE,
>>>+ "Access denied for uid [%"SPRIuid"].\n",
>>>+ client_euid(cctx->creds));
>>> } else {
>>> DEBUG(SSSDBG_OP_FAILURE, "check_allowed_uids failed.\n");
>>> }
>>>diff --git a/src/responder/pam/pamsrv_cmd.c b/src/responder/pam/pamsrv_cmd.c
>>>index 5fe3e6b..bfc534f 100644
>>>--- a/src/responder/pam/pamsrv_cmd.c
>>>+++ b/src/responder/pam/pamsrv_cmd.c
>>>@@ -1001,7 +1001,7 @@ static bool is_uid_trusted(struct cli_creds *creds,
>>> return true;
>>> }
>>>
>>>-ret = check_allowed_uids(creds, trusted_uids_count, trusted_uids);
>>>+ret = check_allowed_uids(client_euid(creds), trusted_uids_count,
>>>trusted_uids);
>>> if (ret == EOK) return true;
>>>
>>> return false;
>>>@@ -1091,13 +1091,13 @@ static int pam_forwarder(struc
[SSSD] Re: [PATCH] Abstract and improve connection credential handling
On 01/21/2016 03:51 PM, Simo Sorce wrote:
On Wed, 2016-01-20 at 16:38 +0100, Lukas Slebodnik wrote:
On (19/01/16 15:38), Simo Sorce wrote:
On Tue, 2016-01-19 at 10:34 -0500, Simo Sorce wrote:
On Tue, 2016-01-19 at 11:23 +0100, Lukas Slebodnik wrote:
[...]
+#endif /* __SSSD_UTIL_SELINUX_H__ */
BTW will we need this header file if we make
struct cli_creds opaque?
Replying to both your mails here:
Making cli_creds opaque requires using a pointer and dealing with
allocating it, I guess I can do that, the headers file would still be
needed in order to avoid huge ifdefs around the functions that implement
handling SELinux stuff. It makes the code a lot more readable and
searchable.
Simo.
Attached find patch that changes the code so that cli_creds is now
opaque.
This *should* work w/o the patch that changes the headers but I haven't
tested it w/o that patch yet.
I had an issue to build it correctly with ifp responder.
src/responder/ifp/ifpsrv_util.c: In function ‘ifp_req_create’:
src/responder/ifp/ifpsrv_util.c:64:30: error: passing argument 1 of
‘check_allowed_uids’ makes pointer from integer without a cast
[-Werror=int-conversion]
ret = check_allowed_uids(dbus_req->client,
^
In file included from ../sssd/src/responder/ifp/ifp_private.h:27:0,
from ../sssd/src/responder/ifp/ifpsrv_util.c:27:
src/responder/common/responder.h:334:9: note: expected ‘struct cli_creds *’ but
argument is of type ‘int64_t {aka long int}’
errno_t check_allowed_uids(struct cli_creds *creds,
^
ifp responder uses different way to obtain uid. I changed back the prototype of
check_allowed_uids.
Here is a diff on to of your patch.
diff --git a/src/responder/common/responder.h b/src/responder/common/responder.h
index 419a863..3b70b69 100644
--- a/src/responder/common/responder.h
+++ b/src/responder/common/responder.h
@@ -331,8 +331,7 @@ errno_t csv_string_to_uid_array(TALLOC_CTX *mem_ctx, const
char *csv_string,
size_t *_uid_count, uid_t **_uids);
uid_t client_euid(struct cli_creds *creds);
-errno_t check_allowed_uids(struct cli_creds *creds,
- size_t allowed_uids_count,
+errno_t check_allowed_uids(uid_t uid, size_t allowed_uids_count,
uid_t *allowed_uids);
struct tevent_req *
diff --git a/src/responder/common/responder_common.c
b/src/responder/common/responder_common.c
index 27193ce..6ac1ea2 100644
--- a/src/responder/common/responder_common.c
+++ b/src/responder/common/responder_common.c
@@ -138,8 +138,7 @@ uid_t client_euid(struct cli_creds *creds)
return cli_creds_get_uid(creds);
}
-errno_t check_allowed_uids(struct cli_creds *creds,
- size_t allowed_uids_count,
+errno_t check_allowed_uids(uid_t uid, size_t allowed_uids_count,
uid_t *allowed_uids)
{
size_t c;
@@ -149,7 +148,7 @@ errno_t check_allowed_uids(struct cli_creds *creds,
}
for (c = 0; c < allowed_uids_count; c++) {
-if (client_euid(creds) == allowed_uids[c]) {
+if (uid == allowed_uids[c]) {
return EOK;
}
}
@@ -449,12 +448,13 @@ static void accept_fd_handler(struct tevent_context *ev,
return;
}
-ret = check_allowed_uids(cctx->creds, rctx->allowed_uids_count,
+ret = check_allowed_uids(client_euid(cctx->creds),
rctx->allowed_uids_count,
rctx->allowed_uids);
if (ret != EOK) {
if (ret == EACCES) {
-DEBUG(SSSDBG_CRIT_FAILURE, "Access denied for uid [%d].\n",
-(int)client_euid(cctx->creds));
+DEBUG(SSSDBG_CRIT_FAILURE,
+ "Access denied for uid [%"SPRIuid"].\n",
+ client_euid(cctx->creds));
} else {
DEBUG(SSSDBG_OP_FAILURE, "check_allowed_uids failed.\n");
}
diff --git a/src/responder/pam/pamsrv_cmd.c b/src/responder/pam/pamsrv_cmd.c
index 5fe3e6b..bfc534f 100644
--- a/src/responder/pam/pamsrv_cmd.c
+++ b/src/responder/pam/pamsrv_cmd.c
@@ -1001,7 +1001,7 @@ static bool is_uid_trusted(struct cli_creds *creds,
return true;
}
-ret = check_allowed_uids(creds, trusted_uids_count, trusted_uids);
+ret = check_allowed_uids(client_euid(creds), trusted_uids_count,
trusted_uids);
if (ret == EOK) return true;
return false;
@@ -1091,13 +1091,13 @@ static int pam_forwarder(struct cli_ctx *cctx, int
pam_cmd)
}
pd = preq->pd;
-preq->is_uid_trusted = is_uid_trusted(&cctx->creds,
+preq->is_uid_trusted = is_uid_trusted(cctx->creds,
pctx->trusted_uids_count,
pctx->trusted_uids);
if (!preq->is_uid_trusted) {
-DEBUG(SSSDBG_MINOR_FAILURE, "uid %d is not trusted.\n",
- (
[SSSD] Re: [PATCH] Abstract and improve connection credential handling
On Wed, 2016-01-20 at 16:38 +0100, Lukas Slebodnik wrote:
> On (19/01/16 15:38), Simo Sorce wrote:
> >On Tue, 2016-01-19 at 10:34 -0500, Simo Sorce wrote:
> >> On Tue, 2016-01-19 at 11:23 +0100, Lukas Slebodnik wrote:
> >> [...]
> >> > >+#endif /* __SSSD_UTIL_SELINUX_H__ */
> >> > BTW will we need this header file if we make
> >> > struct cli_creds opaque?
> >>
> >> Replying to both your mails here:
> >> Making cli_creds opaque requires using a pointer and dealing with
> >> allocating it, I guess I can do that, the headers file would still be
> >> needed in order to avoid huge ifdefs around the functions that implement
> >> handling SELinux stuff. It makes the code a lot more readable and
> >> searchable.
> >>
> >> Simo.
> >>
> >
> >Attached find patch that changes the code so that cli_creds is now
> >opaque.
> >This *should* work w/o the patch that changes the headers but I haven't
> >tested it w/o that patch yet.
> >
> I had an issue to build it correctly with ifp responder.
> src/responder/ifp/ifpsrv_util.c: In function ‘ifp_req_create’:
> src/responder/ifp/ifpsrv_util.c:64:30: error: passing argument 1 of
> ‘check_allowed_uids’ makes pointer from integer without a cast
> [-Werror=int-conversion]
> ret = check_allowed_uids(dbus_req->client,
> ^
> In file included from ../sssd/src/responder/ifp/ifp_private.h:27:0,
> from ../sssd/src/responder/ifp/ifpsrv_util.c:27:
> src/responder/common/responder.h:334:9: note: expected ‘struct cli_creds *’
> but argument is of type ‘int64_t {aka long int}’
> errno_t check_allowed_uids(struct cli_creds *creds,
> ^
>
> ifp responder uses different way to obtain uid. I changed back the prototype
> of
> check_allowed_uids.
> Here is a diff on to of your patch.
>
> diff --git a/src/responder/common/responder.h
> b/src/responder/common/responder.h
> index 419a863..3b70b69 100644
> --- a/src/responder/common/responder.h
> +++ b/src/responder/common/responder.h
> @@ -331,8 +331,7 @@ errno_t csv_string_to_uid_array(TALLOC_CTX *mem_ctx,
> const char *csv_string,
> size_t *_uid_count, uid_t **_uids);
>
> uid_t client_euid(struct cli_creds *creds);
> -errno_t check_allowed_uids(struct cli_creds *creds,
> - size_t allowed_uids_count,
> +errno_t check_allowed_uids(uid_t uid, size_t allowed_uids_count,
> uid_t *allowed_uids);
>
> struct tevent_req *
> diff --git a/src/responder/common/responder_common.c
> b/src/responder/common/responder_common.c
> index 27193ce..6ac1ea2 100644
> --- a/src/responder/common/responder_common.c
> +++ b/src/responder/common/responder_common.c
> @@ -138,8 +138,7 @@ uid_t client_euid(struct cli_creds *creds)
> return cli_creds_get_uid(creds);
> }
>
> -errno_t check_allowed_uids(struct cli_creds *creds,
> - size_t allowed_uids_count,
> +errno_t check_allowed_uids(uid_t uid, size_t allowed_uids_count,
> uid_t *allowed_uids)
> {
> size_t c;
> @@ -149,7 +148,7 @@ errno_t check_allowed_uids(struct cli_creds *creds,
> }
>
> for (c = 0; c < allowed_uids_count; c++) {
> -if (client_euid(creds) == allowed_uids[c]) {
> +if (uid == allowed_uids[c]) {
> return EOK;
> }
> }
> @@ -449,12 +448,13 @@ static void accept_fd_handler(struct tevent_context *ev,
> return;
> }
>
> -ret = check_allowed_uids(cctx->creds, rctx->allowed_uids_count,
> +ret = check_allowed_uids(client_euid(cctx->creds),
> rctx->allowed_uids_count,
> rctx->allowed_uids);
> if (ret != EOK) {
> if (ret == EACCES) {
> -DEBUG(SSSDBG_CRIT_FAILURE, "Access denied for uid [%d].\n",
> -(int)client_euid(cctx->creds));
> +DEBUG(SSSDBG_CRIT_FAILURE,
> + "Access denied for uid [%"SPRIuid"].\n",
> + client_euid(cctx->creds));
> } else {
> DEBUG(SSSDBG_OP_FAILURE, "check_allowed_uids failed.\n");
> }
> diff --git a/src/responder/pam/pamsrv_cmd.c b/src/responder/pam/pamsrv_cmd.c
> index 5fe3e6b..bfc534f 100644
> --- a/src/responder/pam/pamsrv_cmd.c
> +++ b/src/responder/pam/pamsrv_cmd.c
> @@ -1001,7 +1001,7 @@ static bool is_uid_trusted(struct cli_creds *creds,
> return true;
> }
>
> -ret = check_allowed_uids(creds, trusted_uids_count, trusted_uids);
> +ret = check_allowed_uids(client_euid(creds), trusted_uids_count,
> trusted_uids);
> if (ret == EOK) return true;
>
> return false;
> @@ -1091,13 +1091,13 @@ static int pam_forwarder(struct cli_ctx *cctx, int
> pam_cmd)
> }
> pd = preq->pd;
>
> -preq->is_uid_trusted = is_uid_trusted(&cctx->creds,
> +preq->is_uid_trusted = is_uid_trusted(cctx->creds,
>
[SSSD] Re: [PATCH] Abstract and improve connection credential handling
On (19/01/16 15:38), Simo Sorce wrote:
>On Tue, 2016-01-19 at 10:34 -0500, Simo Sorce wrote:
>> On Tue, 2016-01-19 at 11:23 +0100, Lukas Slebodnik wrote:
>> [...]
>> > >+#endif /* __SSSD_UTIL_SELINUX_H__ */
>> > BTW will we need this header file if we make
>> > struct cli_creds opaque?
>>
>> Replying to both your mails here:
>> Making cli_creds opaque requires using a pointer and dealing with
>> allocating it, I guess I can do that, the headers file would still be
>> needed in order to avoid huge ifdefs around the functions that implement
>> handling SELinux stuff. It makes the code a lot more readable and
>> searchable.
>>
>> Simo.
>>
>
>Attached find patch that changes the code so that cli_creds is now
>opaque.
>This *should* work w/o the patch that changes the headers but I haven't
>tested it w/o that patch yet.
>
I had an issue to build it correctly with ifp responder.
src/responder/ifp/ifpsrv_util.c: In function ‘ifp_req_create’:
src/responder/ifp/ifpsrv_util.c:64:30: error: passing argument 1 of
‘check_allowed_uids’ makes pointer from integer without a cast
[-Werror=int-conversion]
ret = check_allowed_uids(dbus_req->client,
^
In file included from ../sssd/src/responder/ifp/ifp_private.h:27:0,
from ../sssd/src/responder/ifp/ifpsrv_util.c:27:
src/responder/common/responder.h:334:9: note: expected ‘struct cli_creds *’ but
argument is of type ‘int64_t {aka long int}’
errno_t check_allowed_uids(struct cli_creds *creds,
^
ifp responder uses different way to obtain uid. I changed back the prototype of
check_allowed_uids.
Here is a diff on to of your patch.
diff --git a/src/responder/common/responder.h b/src/responder/common/responder.h
index 419a863..3b70b69 100644
--- a/src/responder/common/responder.h
+++ b/src/responder/common/responder.h
@@ -331,8 +331,7 @@ errno_t csv_string_to_uid_array(TALLOC_CTX *mem_ctx, const
char *csv_string,
size_t *_uid_count, uid_t **_uids);
uid_t client_euid(struct cli_creds *creds);
-errno_t check_allowed_uids(struct cli_creds *creds,
- size_t allowed_uids_count,
+errno_t check_allowed_uids(uid_t uid, size_t allowed_uids_count,
uid_t *allowed_uids);
struct tevent_req *
diff --git a/src/responder/common/responder_common.c
b/src/responder/common/responder_common.c
index 27193ce..6ac1ea2 100644
--- a/src/responder/common/responder_common.c
+++ b/src/responder/common/responder_common.c
@@ -138,8 +138,7 @@ uid_t client_euid(struct cli_creds *creds)
return cli_creds_get_uid(creds);
}
-errno_t check_allowed_uids(struct cli_creds *creds,
- size_t allowed_uids_count,
+errno_t check_allowed_uids(uid_t uid, size_t allowed_uids_count,
uid_t *allowed_uids)
{
size_t c;
@@ -149,7 +148,7 @@ errno_t check_allowed_uids(struct cli_creds *creds,
}
for (c = 0; c < allowed_uids_count; c++) {
-if (client_euid(creds) == allowed_uids[c]) {
+if (uid == allowed_uids[c]) {
return EOK;
}
}
@@ -449,12 +448,13 @@ static void accept_fd_handler(struct tevent_context *ev,
return;
}
-ret = check_allowed_uids(cctx->creds, rctx->allowed_uids_count,
+ret = check_allowed_uids(client_euid(cctx->creds),
rctx->allowed_uids_count,
rctx->allowed_uids);
if (ret != EOK) {
if (ret == EACCES) {
-DEBUG(SSSDBG_CRIT_FAILURE, "Access denied for uid [%d].\n",
-(int)client_euid(cctx->creds));
+DEBUG(SSSDBG_CRIT_FAILURE,
+ "Access denied for uid [%"SPRIuid"].\n",
+ client_euid(cctx->creds));
} else {
DEBUG(SSSDBG_OP_FAILURE, "check_allowed_uids failed.\n");
}
diff --git a/src/responder/pam/pamsrv_cmd.c b/src/responder/pam/pamsrv_cmd.c
index 5fe3e6b..bfc534f 100644
--- a/src/responder/pam/pamsrv_cmd.c
+++ b/src/responder/pam/pamsrv_cmd.c
@@ -1001,7 +1001,7 @@ static bool is_uid_trusted(struct cli_creds *creds,
return true;
}
-ret = check_allowed_uids(creds, trusted_uids_count, trusted_uids);
+ret = check_allowed_uids(client_euid(creds), trusted_uids_count,
trusted_uids);
if (ret == EOK) return true;
return false;
@@ -1091,13 +1091,13 @@ static int pam_forwarder(struct cli_ctx *cctx, int
pam_cmd)
}
pd = preq->pd;
-preq->is_uid_trusted = is_uid_trusted(&cctx->creds,
+preq->is_uid_trusted = is_uid_trusted(cctx->creds,
pctx->trusted_uids_count,
pctx->trusted_uids);
if (!preq->is_uid_trusted) {
-DEBUG(SSSDBG_MINOR_FAILURE, "uid %d is not trusted.\n",
- (int)client_euid(&cctx->creds));
+DEBUG(SSSDBG_MINOR_FAILURE, "uid %"SP
[SSSD] Re: [PATCH] Abstract and improve connection credential handling
On Tue, 2016-01-19 at 10:34 -0500, Simo Sorce wrote:
> On Tue, 2016-01-19 at 11:23 +0100, Lukas Slebodnik wrote:
> [...]
> > >+#endif /* __SSSD_UTIL_SELINUX_H__ */
> > BTW will we need this header file if we make
> > struct cli_creds opaque?
>
> Replying to both your mails here:
> Making cli_creds opaque requires using a pointer and dealing with
> allocating it, I guess I can do that, the headers file would still be
> needed in order to avoid huge ifdefs around the functions that implement
> handling SELinux stuff. It makes the code a lot more readable and
> searchable.
>
> Simo.
>
Attached find patch that changes the code so that cli_creds is now
opaque.
This *should* work w/o the patch that changes the headers but I haven't
tested it w/o that patch yet.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
From 47d2ab3bb559ed0b0ea112c2eb8b3c4ceaa86929 Mon Sep 17 00:00:00 2001
From: Simo Sorce
Date: Mon, 18 Jan 2016 15:21:57 -0500
Subject: [PATCH] Util: Improve code to get connection credentials
Adds support to get SELINUX context and make code more abstract so
that struct ucred (if availale) can be used w/o redefining uid,gid,pid to
int32. Also givces a layer of indirection that may come handy if we want
to imrpove the code further in the future.
Resolves:
https://fedorahosted.org/sssd/ticket/
---
Makefile.am | 2 +
src/responder/common/responder.h| 11 +++--
src/responder/common/responder_common.c | 59
src/responder/pam/pamsrv_cmd.c | 23 -
src/util/util_creds.h | 82 +
5 files changed, 141 insertions(+), 36 deletions(-)
create mode 100644 src/util/util_creds.h
diff --git a/Makefile.am b/Makefile.am
index 407053b1a6dcd0255be76ae7f9252a671b965ea2..98f5df9ff4fbb122930c6109620a58cfd1ca 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -496,6 +496,7 @@ SSSD_LIBS = \
$(COLLECTION_LIBS) \
$(DHASH_LIBS) \
$(OPENLDAP_LIBS) \
+$(SELINUX_LIBS) \
$(TDB_LIBS)
PYTHON_BINDINGS_LIBS = \
@@ -556,6 +557,7 @@ dist_noinst_HEADERS = \
src/util/authtok-utils.h \
src/util/util_safealign.h \
src/util/util_sss_idmap.h \
+src/util/util_creds.h \
src/monitor/monitor.h \
src/monitor/monitor_interfaces.h \
src/responder/common/responder.h \
diff --git a/src/responder/common/responder.h b/src/responder/common/responder.h
index 6b511368c9b08d1cfc828d16f57a2cde902dc82b..69eb6a1888a0c3632fc61768e12ce60c0114d22b 100644
--- a/src/responder/common/responder.h
+++ b/src/responder/common/responder.h
@@ -123,6 +123,8 @@ struct resp_ctx {
bool shutting_down;
};
+struct cli_creds;
+
struct cli_ctx {
struct tevent_context *ev;
struct resp_ctx *rctx;
@@ -131,9 +133,8 @@ struct cli_ctx {
tevent_fd_handler_t cfd_handler;
struct sockaddr_un addr;
int priv;
-int32_t client_euid;
-int32_t client_egid;
-int32_t client_pid;
+
+struct cli_creds *creds;
void *protocol_ctx;
void *state_ctx;
@@ -331,7 +332,9 @@ errno_t csv_string_to_uid_array(TALLOC_CTX *mem_ctx, const char *csv_string,
bool allow_sss_loop,
size_t *_uid_count, uid_t **_uids);
-errno_t check_allowed_uids(uid_t uid, size_t allowed_uids_count,
+uid_t client_euid(struct cli_creds *creds);
+errno_t check_allowed_uids(struct cli_creds *creds,
+ size_t allowed_uids_count,
uid_t *allowed_uids);
struct tevent_req *
diff --git a/src/responder/common/responder_common.c b/src/responder/common/responder_common.c
index fecb72384786ea2840ed349a6afcb36efc60d492..fc074fee25f96a931649d39cccaa4fd8312ee4e7 100644
--- a/src/responder/common/responder_common.c
+++ b/src/responder/common/responder_common.c
@@ -42,6 +42,7 @@
#include "providers/data_provider.h"
#include "monitor/monitor_interfaces.h"
#include "sbus/sbus_client.h"
+#include "util/util_creds.h"
#ifdef HAVE_SYSTEMD
#include
@@ -88,16 +89,20 @@ static int client_destructor(struct cli_ctx *ctx)
static errno_t get_client_cred(struct cli_ctx *cctx)
{
-cctx->client_euid = -1;
-cctx->client_egid = -1;
-cctx->client_pid = -1;
+SEC_CTX secctx;
+int ret;
+
+cctx->creds = talloc(cctx, struct cli_creds);
+if (!cctx->creds) return ENOMEM;
#ifdef HAVE_UCRED
-int ret;
-struct ucred client_cred;
-socklen_t client_cred_len = sizeof(client_cred);
+socklen_t client_cred_len = sizeof(struct ucred);
-ret = getsockopt(cctx->cfd, SOL_SOCKET, SO_PEERCRED, &client_cred,
+cctx->creds->ucred.uid = -1;
+cctx->creds->ucred.gid = -1;
+cctx->creds->ucred.pid = -1;
+
+ret = getsockopt(cctx->cfd, SOL_SOCKET, SO_PEERCRED, &cctx->creds->ucred,
&client_cred_len);
if (ret != EOK) {
ret = errno;
@@ -111,18 +116,34 @@ static errno_t get_client_cred(struct cli_ctx *cctx)
return ENOMS
[SSSD] Re: [PATCH] Abstract and improve connection credential handling
On Tue, 2016-01-19 at 11:23 +0100, Lukas Slebodnik wrote: [...] > >+#endif /* __SSSD_UTIL_SELINUX_H__ */ > BTW will we need this header file if we make > struct cli_creds opaque? Replying to both your mails here: Making cli_creds opaque requires using a pointer and dealing with allocating it, I guess I can do that, the headers file would still be needed in order to avoid huge ifdefs around the functions that implement handling SELinux stuff. It makes the code a lot more readable and searchable. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] Abstract and improve connection credential handling
On (18/01/16 18:06), Simo Sorce wrote:
>I will needed selinux support later on in the secrets work, so I looked
>into how we were getting peercreds and found it could be improved by
>making it a little more abstract.
>
>This patch also uncovered issues with header inclusion (patch sent
>earlier).
>
>(I did not open a bug for this one)
>
>Simo.
>
>--
>Simo Sorce * Red Hat, Inc * New York
>From 7cc82eff48dabc4b15e119146f36597f4cd75827 Mon Sep 17 00:00:00 2001
>From: Simo Sorce
>Date: Mon, 18 Jan 2016 15:21:57 -0500
>Subject: [PATCH] Util: Improve code to get connection credentials
>
>Adds support to get SELINUX context and make code more abstract so
>that struct ucred (if availale) can be used w/o redefining uid,gid,pid to
>int32. Also givces a layer of indirection that may come handy if we want
>to imrpove the code further in the future.
>
>Resolves:
>https://fedorahosted.org/sssd/ticket/
>---
> Makefile.am | 2 +
> src/responder/common/responder.h| 25 ++--
> src/responder/common/responder_common.c | 50 +++-
> src/responder/pam/pamsrv_cmd.c | 23 +--
> src/util/util_selinux.h | 68 +
> 5 files changed, 133 insertions(+), 35 deletions(-)
> create mode 100644 src/util/util_selinux.h
>
>diff --git a/Makefile.am b/Makefile.am
>index
>407053b1a6dcd0255be76ae7f9252a671b965ea2..2a02add0dc1942c57dec03f4762444c48a710a10
> 100644
>--- a/Makefile.am
>+++ b/Makefile.am
>@@ -496,6 +496,7 @@ SSSD_LIBS = \
> $(COLLECTION_LIBS) \
> $(DHASH_LIBS) \
> $(OPENLDAP_LIBS) \
>+$(SELINUX_LIBS) \
> $(TDB_LIBS)
>
> PYTHON_BINDINGS_LIBS = \
>@@ -556,6 +557,7 @@ dist_noinst_HEADERS = \
> src/util/authtok-utils.h \
> src/util/util_safealign.h \
> src/util/util_sss_idmap.h \
>+src/util/util_selinux.h \
> src/monitor/monitor.h \
> src/monitor/monitor_interfaces.h \
> src/responder/common/responder.h \
>diff --git a/src/responder/common/responder.h
>b/src/responder/common/responder.h
>index
>6b511368c9b08d1cfc828d16f57a2cde902dc82b..4735eb7af7d65c1e822d662e7200a8a7418e191e
> 100644
>--- a/src/responder/common/responder.h
>+++ b/src/responder/common/responder.h
>@@ -36,6 +36,7 @@
> #include "sbus/sssd_dbus.h"
> #include "responder/common/negcache.h"
> #include "sss_client/sss_cli.h"
>+#include "util/util_selinux.h"
>
> extern hash_table_t *dp_requests;
>
>@@ -123,6 +124,21 @@ struct resp_ctx {
> bool shutting_down;
> };
>
>+#ifdef HAVE_UCRED
>+typedef struct ucred UCRED_CTX;
>+#define UCRED_get_uid(x) x.uid
>+
>+#else /* not HAVE_UCRED */
>+struct ucred_ctx { int none; };
>+typedef struct ucred_ctx UCRED_CTX;
>+#define UCRED_get_uid(x) -1
>+#endif /* done HAVE_UCRED */
>+
>+struct cli_creds {
>+UCRED_CTX ucred;
>+SELINUX_CTX selinux_ctx;
>+};
>+
> struct cli_ctx {
> struct tevent_context *ev;
> struct resp_ctx *rctx;
>@@ -131,9 +147,8 @@ struct cli_ctx {
> tevent_fd_handler_t cfd_handler;
> struct sockaddr_un addr;
> int priv;
>-int32_t client_euid;
>-int32_t client_egid;
>-int32_t client_pid;
>+
>+struct cli_creds creds;
>
> void *protocol_ctx;
> void *state_ctx;
>@@ -331,7 +346,9 @@ errno_t csv_string_to_uid_array(TALLOC_CTX *mem_ctx, const
>char *csv_string,
> bool allow_sss_loop,
> size_t *_uid_count, uid_t **_uids);
>
>-errno_t check_allowed_uids(uid_t uid, size_t allowed_uids_count,
>+uid_t client_euid(struct cli_creds *creds);
>+errno_t check_allowed_uids(struct cli_creds *creds,
>+ size_t allowed_uids_count,
>uid_t *allowed_uids);
>
> struct tevent_req *
>diff --git a/src/responder/common/responder_common.c
>b/src/responder/common/responder_common.c
>index
>fecb72384786ea2840ed349a6afcb36efc60d492..1417c600e35afab6298109d8be6b6dc14b4b51eb
> 100644
>--- a/src/responder/common/responder_common.c
>+++ b/src/responder/common/responder_common.c
>@@ -88,16 +88,17 @@ static int client_destructor(struct cli_ctx *ctx)
>
> static errno_t get_client_cred(struct cli_ctx *cctx)
> {
>-cctx->client_euid = -1;
>-cctx->client_egid = -1;
>-cctx->client_pid = -1;
>+SEC_CTX secctx;
>+int ret;
>
> #ifdef HAVE_UCRED
>-int ret;
>-struct ucred client_cred;
>-socklen_t client_cred_len = sizeof(client_cred);
>+socklen_t client_cred_len = sizeof(struct ucred);
>
>-ret = getsockopt(cctx->cfd, SOL_SOCKET, SO_PEERCRED, &client_cred,
>+cctx->creds.ucred.uid = -1;
>+cctx->creds.ucred.gid = -1;
>+cctx->creds.ucred.pid = -1;
>+
>+ret = getsockopt(cctx->cfd, SOL_SOCKET, SO_PEERCRED, &cctx->creds.ucred,
> &client_cred_len);
> if (ret != EOK) {
> ret = errno;
>@@ -111,18 +112,31 @@ static errno_t get_client_cred(struct cli_ctx *cctx)
> return ENOMSG;
> }
>
>-cctx->client_euid = client_cred.uid;
>-
[SSSD] Re: [PATCH] Abstract and improve connection credential handling
On (18/01/16 18:06), Simo Sorce wrote:
>I will needed selinux support later on in the secrets work, so I looked
>into how we were getting peercreds and found it could be improved by
>making it a little more abstract.
>
>This patch also uncovered issues with header inclusion (patch sent
>earlier).
>
>(I did not open a bug for this one)
>
>Simo.
>
>--
>Simo Sorce * Red Hat, Inc * New York
>From 7cc82eff48dabc4b15e119146f36597f4cd75827 Mon Sep 17 00:00:00 2001
>From: Simo Sorce
>Date: Mon, 18 Jan 2016 15:21:57 -0500
>Subject: [PATCH] Util: Improve code to get connection credentials
>
>Adds support to get SELINUX context and make code more abstract so
>that struct ucred (if availale) can be used w/o redefining uid,gid,pid to
>int32. Also givces a layer of indirection that may come handy if we want
>to imrpove the code further in the future.
>
>Resolves:
>https://fedorahosted.org/sssd/ticket/
>---
> Makefile.am | 2 +
> src/responder/common/responder.h| 25 ++--
> src/responder/common/responder_common.c | 50 +++-
> src/responder/pam/pamsrv_cmd.c | 23 +--
> src/util/util_selinux.h | 68 +
> 5 files changed, 133 insertions(+), 35 deletions(-)
> create mode 100644 src/util/util_selinux.h
>
>diff --git a/Makefile.am b/Makefile.am
>index
>407053b1a6dcd0255be76ae7f9252a671b965ea2..2a02add0dc1942c57dec03f4762444c48a710a10
> 100644
>--- a/Makefile.am
>+++ b/Makefile.am
>@@ -496,6 +496,7 @@ SSSD_LIBS = \
> $(COLLECTION_LIBS) \
> $(DHASH_LIBS) \
> $(OPENLDAP_LIBS) \
>+$(SELINUX_LIBS) \
> $(TDB_LIBS)
>
> PYTHON_BINDINGS_LIBS = \
>@@ -556,6 +557,7 @@ dist_noinst_HEADERS = \
> src/util/authtok-utils.h \
> src/util/util_safealign.h \
> src/util/util_sss_idmap.h \
>+src/util/util_selinux.h \
> src/monitor/monitor.h \
> src/monitor/monitor_interfaces.h \
> src/responder/common/responder.h \
>diff --git a/src/responder/common/responder.h
>b/src/responder/common/responder.h
>index
>6b511368c9b08d1cfc828d16f57a2cde902dc82b..4735eb7af7d65c1e822d662e7200a8a7418e191e
> 100644
>--- a/src/responder/common/responder.h
>+++ b/src/responder/common/responder.h
>@@ -36,6 +36,7 @@
> #include "sbus/sssd_dbus.h"
> #include "responder/common/negcache.h"
> #include "sss_client/sss_cli.h"
>+#include "util/util_selinux.h"
>
> extern hash_table_t *dp_requests;
>
>@@ -123,6 +124,21 @@ struct resp_ctx {
> bool shutting_down;
> };
>
>+#ifdef HAVE_UCRED
>+typedef struct ucred UCRED_CTX;
>+#define UCRED_get_uid(x) x.uid
>+
>+#else /* not HAVE_UCRED */
>+struct ucred_ctx { int none; };
>+typedef struct ucred_ctx UCRED_CTX;
>+#define UCRED_get_uid(x) -1
>+#endif /* done HAVE_UCRED */
>+
>+struct cli_creds {
>+UCRED_CTX ucred;
>+SELINUX_CTX selinux_ctx;
>+};
>+
Here is a small issue.
The header file "src/responder/common/responder.h" create typedef
for "struct ucred". This structure is defined only for "__USE_GNU"
in heder file "bits/socket.h". This system header file can
be included by any other system file. e.g. sys/time.h
Therefore it will work for you if you include "util/util.h"
as the first header file. Because it includes config.h
which enable GNU features. But you need to do in all
implementation modules which inlcude directly or indirectly
"src/responder/common/responder.h". The main problem is than
most of responder modules does not use cli_creds.
sh$ $git grep cli_creds
src/responder/common/responder.h:struct cli_creds {
src/responder/common/responder.h:struct cli_creds creds;
src/responder/common/responder.h:uid_t client_euid(struct cli_creds *creds);
src/responder/common/responder.h:errno_t check_allowed_uids(struct cli_creds
*creds,
src/responder/common/responder_common.c:uid_t client_euid(struct cli_creds
*creds)
src/responder/common/responder_common.c:errno_t check_allowed_uids(struct
cli_creds *creds,
src/responder/pam/pamsrv_cmd.c:static bool is_uid_trusted(struct cli_creds
*creds,
sh$ git grep client_euid
src/responder/common/responder.h:uid_t client_euid(struct cli_creds *creds);
src/responder/common/responder_common.c:uid_t client_euid(struct cli_creds
*creds)
src/responder/common/responder_common.c:if (client_euid(&cctx->creds)
== -1) {
src/responder/common/responder_common.c:
(int)client_euid(&cctx->creds));
src/responder/pam/pamsrv_cmd.c:if (client_euid(creds) == 0) {
src/responder/pam/pamsrv_cmd.c: (int)client_euid(&cctx->creds));
src/responder/pam/pamsrv_cmd.c:
(int)client_euid(&preq->cctx->creds), preq->pd->domain);
sh$ grep check_allowed_uids
src/responder/common/responder.h:errno_t check_allowed_uids(struct cli_creds
*creds,
src/responder/common/responder_common.c:errno_t check_allowed_uids(struct
cli_creds *creds,
src/responder/common/responder_common.c:ret =
check_allowed_uids(&cctx->creds, rctx->allowed_uids_count,
src
