[SSSD] [PATCHES] Support one-way trusts with AD domains in IPA server mode

Jakub Hrozek jhrozek at redhat.com
Wed Jun 10 20:41:12 UTC 2015


On Tue, Jun 09, 2015 at 01:22:20PM +0200, Sumit Bose wrote:
> =================================
> IPA: Fetch keytab for 1way trusts
> 
> Why do you have to wrap execl(), looks like only execle() is used?

You're right, I don't. I just used execl first, then converted to
execle() because I need to set the right KRB5CCNAME, but forgot to
remove the old wrapper after testing..

Thanks, removed.

> 
> > +    errno_t ret;
> > +    struct tevent_req *req = NULL;
> > +    struct ipa_getkeytab_state *state;
> > +    pid_t child_pid;
> > +    struct timeval tv;
> > +
> > +    req = tevent_req_create(mem_ctx, &state, struct ipa_getkeytab_state);
> > +    if (req == NULL) {
> > +        return NULL;
> > +    }
> > +    state->child_status = 0;
> 
> Maybe it would be safer to set this to a generic error like EFAULT to make sure
> 0/EOK is only returned if the request is successful?

Yes, good idea. As you noted, there is a bit of copy-paste in this
area..

> 
> > +
> > +    if (server == NULL || principal == NULL || keytab == NULL) {
> > +        ret = EINVAL;
> > +        goto done;
> > +    }
> 
> Just a general comment, maybe it would help to reduce code duplication if
> child_handler_setup() would create the timeout handler as well? Shall we add a
> ticket?

Yes: https://fedorahosted.org/sssd/ticket/2674

> 
> > +    gkt_env[0] = talloc_asprintf(NULL, "KRB5CCNAME=%s", ccache);
> > +    if (gkt_env[0] == NULL) {
> > +        DEBUG(SSSDBG_CRIT_FAILURE, "OOM\n");
> 
> Since SSSDBG_CRIT_FAILURE messages are expected to be read by users/admins I
> think it should be a bit more explicit.

Yes, sorry. I initially wrote this part of code as a quick for for ab
and then just squashed into the patchset.

> 
> > +        exit(1);
> > +    }
> > +
> > +    errno = 0;
> > +    execle(IPA_GETKEYTAB_PATH, IPA_GETKEYTAB_PATH,
> > +           "-r", "-s", server, "-p", principal, "-k", keytab_path, NULL,
> > +           gkt_env);
> 
> It's academic but I guess catching the return value of execle() would help to
> avoid some complaints from static code-analyzers.

OK, to avoid static code-analyzers from complaining that the value of
ret is read, but never used, I also added another DEBUG message.

> 
> > +
> > +    /* The child should never end up here */
> > +    ret = errno;
> > +    DEBUG(SSSDBG_CRIT_FAILURE,
> > +          "execv failed [%d][%s].\n", ret, sss_strerror(ret));
> > +    exit(1);
> 
> ======================================================
> AD: Rename ad_set_ad_id_options to ad_set_sdap_options
> 
> ACK

RB: you

> 
> ====================================================================
> AD: Rename ad_create_default_options to ad_create_2way_trust_options
> 
> ACK

RB: you

> 
> =======================================
> AD: Split off ad_create_default_options
> 
> > +static void test_ad_create_default_options(void **state)
> > +{
> > +    struct ad_options *ad_options;
> > +    const char *s;
> > +
> > +    ad_options = ad_create_default_options(global_talloc_context);
> > +
> > +    assert_non_null(ad_options->basic);
> > +
> > +    /* Not too much to test here except some defaults */
> > +    s = dp_opt_get_string(ad_options->basic, AD_DOMAIN);
> > +    assert_null(s);
> 
> Maybe you can check the value as well?

The default domain is NULL..

maybe we could check other parameters, like that we default to GC
first..

> 
> > +
> > +    assert_non_null(ad_options->id);
> > +}
> 
> ========================================================
> IPA/AD: Set up AD domain in ad_create_2way_trust_options
> 
> ACK

RB: you

> 
> ===================================
> IPA: Do not set AD_KRB5_REALM twice
> 
> ACK

RB: you

> 
> ====================================
> AD: Add ad_create_1way_trust_options
> 
> Typo in the commit message 'unline'.

Fixed and added RB: you

> 
> ==========================================================
> IPA: Utility function for setting up one-way trust context
> 
> > +    ad_options = ad_create_1way_trust_options(id_ctx,
> > +                                              ad_domain,
> > +                                              id_ctx->server_mode->hostname,
> > +                                              keytab,
> > +                                              principal);
> > +    if (ad_options == NULL) {
> > +        talloc_free(keytab);
> > +        talloc_free(principal);
> > +        return NULL;
> > +    }
> > +
> > +    talloc_steal(ad_options, keytab);
> > +    talloc_steal(ad_options, principal);
> 
> Why do you have to keep keytab and principal around? They should have been
> talloc_strdup'd in ad_create_1way_trust_options.

Yes, I don't think we do.

> 
> > +    return ad_options;
> 
> ================================================================================
> WORKAROUND: setting keytab through environment variable is no longer a good idea
> 
> vs
> 
> remove setenvs
> 
> Which is the right one?

Neither, I didn't mean to send them for review in the current state.

But for Alpha, I will prepare a patch that does setenv/unsetenv and for
Beta, I'll finish testing ccache collections.

> 
> ===========================================================
> LDAP: Consolidate SDAP_SASL_REALM/SDAP_KRB5_REALM behaviour
> 
> ACK

Ah, thanks, I didn't mean to send this one for review yet, but I'm glad
you like it.

I pushed new patches into my fedorapeople branch. I haven't done any
testing of the modifications except for make check -- that will come
tomorrow.

Thanks again for the review.



More information about the sssd-devel mailing list