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'
See logs here:
http://sssd-ci.duckdns.org/logs/job/36/66/debian_testing/ci-build-debug/c...
Michal