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,
>>>> 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 %"SPRIuid" is not
trusted.\n",
>>>>+ client_euid(cctx->creds));
>>>> }
>>>>
>>>>
>>>>@@ -1776,8 +1776,8 @@ static void pam_dom_forwarder(struct pam_auth_req
*preq)
>>>> !is_domain_public(preq->pd->domain,
pctx->public_domains,
>>>> pctx->public_domains_count)) {
>>>> DEBUG(SSSDBG_MINOR_FAILURE,
>>>>- "Untrusted user %d cannot access non-public domain
%s.\n",
>>>>- (int)client_euid(&preq->cctx->creds),
preq->pd->domain);
>>>>+ "Untrusted user %"SPRIuid" cannot access
non-public domain %s.\n",
>>>>+ client_euid(preq->cctx->creds),
preq->pd->domain);
>>>> preq->pd->pam_status = PAM_PERM_DENIED;
>>>> pam_reply(preq);
>>>> return;
>>>>
>>>>
>>>>
>>>>I know you are busy. I fixed few conflics + updated your patch and
>>>>added my singoff. I hope you don mind.
>>>>
>>>>If you are find with attached patch then we need to find 3rd person
>>>>for review (and ACK)
>>>
>>>LGTM
>>>
>>>Simo.
>>>
>>
>>Hi!
>>
>>Lukas, could you please rebase the patch on top of current master?
>>
>There was a small conflict in Makefile.am which could be easily solved by
>3-way megre and should not have blocked a review.
>
>Updated patch is attached.
>
>LS
>
With this patch, the cmocka tests do not link with
libselinux on Debian which results in:
undefined reference to symbol 'freecon'
Updated patch is attached.