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