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
>>>>>>> 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.
>>
>
>I did some manual testing of selinux functionality with FreeIPA
>server. The patch works with what I tried. The code looks good
>to me and CI passed.
>
>CI link:
>http://sssd-ci.duckdns.org/logs/job/36/68/summary.html
>
>ACK.
>
master:
* 6499d0b915209b670f8e337c4fe76a8be9fa6576
LS
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
