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.