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.
Michal