[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