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