On Fri, Jul 31, 2015 at 11:34:23AM +0200, Sumit Bose wrote:
> Hi,
>
> it turned out that some of the current SSSD behaviour does not fit well
> if a KDC proxy is configured, see
>
https://fedorahosted.org/sssd/ticket/2652 and
>
https://fedorahosted.org/sssd/ticket/2700 for details.
>
> The first patch in this series introduces a new call which checks if a
> KDC proxy is configured as suggested in the tickets. The other two
> patches aim to fix the respective ticket.
>
> bye,
> Sumit
>
Hi,
this is not a self-review, I just want the other developers to check
something which I think looks odd.
> diff --git a/src/providers/krb5/krb5_child_handler.c
b/src/providers/krb5/krb5_child_handler.c
> index
4e453b02d5f4e57b8f5a8f48d7be2434654c0f7e..da5d2707894511a97b7f13eef888f359907b41e0 100644
> --- a/src/providers/krb5/krb5_child_handler.c
> +++ b/src/providers/krb5/krb5_child_handler.c
> @@ -177,7 +177,12 @@ static errno_t create_send_buffer(struct krb5child_req *kr,
> SAFEALIGN_COPY_UINT32(&buf->data[rp], &kr->uid, &rp);
> SAFEALIGN_COPY_UINT32(&buf->data[rp], &kr->gid, &rp);
> SAFEALIGN_COPY_UINT32(&buf->data[rp], &validate, &rp);
> - SAFEALIGN_COPY_UINT32(&buf->data[rp], &kr->is_offline, &rp);
> + if (sss_krb5_realm_has_proxy(dp_opt_get_cstring(kr->krb5_ctx->opts,
> + KRB5_REALM))) {
> + SAFEALIGN_SET_UINT32(&buf->data[rp], 0, &rp);
> + } else {
> + SAFEALIGN_COPY_UINT32(&buf->data[rp], &kr->is_offline,
&rp);
I didn't change the line above, just moved it. is_offline is defined as
bool in struct krb5child_req. I think we were just lucky the we didn't
run into issues here so far. I think it would be more clean to use
something like:
SAFEALIGN_SET_UINT32(&buf->data[rp], (kr->is_offline ? 1 : 0), &rp);
to make sure we do not depend on memory layout. I haven't modified it in
the patch because I didn't want to change the existing behaviour.
I agree we should change it. btw on the krb5_child.c side, it's uint32_t
as appropriate.