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.