On Fri, Jan 15, 2016 at 04:44:55PM +0100, Jakub Hrozek wrote:
On Fri, Jan 15, 2016 at 10:44:44AM +0100, Sumit Bose wrote:
> On Thu, Jan 14, 2016 at 05:54:06PM +0100, Sumit Bose wrote:
> > Hi,
> >
> > this patch adds a task to the AD provider which calls adcli on a regular
> > basis to update the machine account password if needed. adcli supports
> > this functionality since version 0.8.0. Adding support other utilities like
> > msktutil shouldn't be hard.
> >
> > Since adcli (and other external tools) does not understand the SSSD
> > default options like --debug_level a change in exec_child_ex() was
> > needed which is covered in the first patch.
> >
> > bye,
> > Sumit
>
> patches are now rebased on current master.
>
> bye,
> Sumit
> From eee0ca691dbf19942b479122e14f413b45b2ba39 Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose(a)redhat.com>
> Date: Tue, 12 Jan 2016 11:05:02 +0100
> Subject: [PATCH 2/2] AD: add task to renew the machine account password if
> needed
>
> AD expects its clients to renew the machine account password on a
> regular basis, be default every 30 days. Even if a client does not renew
> the password it might not cause issues because AD does not enforce the
> renewal. But the password age might be used to identify unused machine
> accounts in large environments which might get disabled or deleted
> automatically.
>
> With this patch SSSD calls an external program to check the age of the
> machine account password and renew it if needed. Currently 'adcli' is
> used as external program which is able to renew the password since
> version 0.8.0.
[...]
> +static errno_t get_adcli_extra_args(struct renewal_data *renewal_data)
> +{
> + const char **args;
> + size_t c = 0;
> +
> + renewal_data->prog_path = talloc_strdup(renewal_data, RENEWAL_PROG_PATH);
We don't need to strdup a constant..
yes, but I prefer to have it allocated to make it easier to switch to
other external tools based on the configuration. If you think it is
currently a total waste of memory I'd rather just user RENEWAL_PROG_PATH
in the parameter list of exec_child_ex() for the time being.
> + if (renewal_data->prog_path == NULL) {
> + DEBUG(SSSDBG_OP_FAILURE, "talloc_strdup failed.\n");
> + return ENOMEM;
> + }
> +
> + args = talloc_array(renewal_data, const char *, 7);
[...]
> +static void ad_machine_account_password_renewal_done(struct tevent_req *subreq)
> +{
> + uint8_t *buf;
> + ssize_t buf_len;
> + struct tevent_req *req = tevent_req_callback_data(subreq,
> + struct tevent_req);
> + struct renewal_state *state = tevent_req_data(req, struct renewal_state);
> + int ret;
> +
> + talloc_zfree(state->timeout_handler);
> +
> + ret = read_pipe_recv(subreq, state, &buf, &buf_len);
> + talloc_zfree(subreq);
> + if (ret != EOK) {
> + tevent_req_error(req, ret);
> + return;
> + }
> +
> + DEBUG(SSSDBG_TRACE_LIBS, "--- adcli output start---\n"
> + "%.*s"
> + "---adcli output end---\n",
> + buf_len, buf);
I didn't see this myself, but coverity detected an issue here:
sssd-1.13.90/src/providers/ad/ad_machine_pw_renewal.c: scope_hint: In function
'ad_machine_account_password_renewal_done'
sssd-1.13.90/src/providers/ad/ad_machine_pw_renewal.c:237:5: warning: field precision
specifier '.*' expects argument of type 'int', but argument 6 has type
'ssize_t' [-Wformat=]
# DEBUG(SSSDBG_TRACE_LIBS, "--- adcli output start---\n"
# ^
# 235| }
# 236|
# 237|-> DEBUG(SSSDBG_TRACE_LIBS, "--- adcli output start---\n"
# 238| "%.*s"
# 239| "---adcli output end---\n",
I added an explicit cast as Lukas suggested.
> +
> + close(state->read_from_child_fd);
> + state->read_from_child_fd = -1;
> +
> +
> + tevent_req_done(req);
> + return;
> +}
> +
> +static void
> +ad_machine_account_password_renewal_timeout(struct tevent_context *ev,
> + struct tevent_timer *te,
> + struct timeval tv, void *pvt)
> +{
> + struct tevent_req *req = talloc_get_type(pvt, struct tevent_req);
> + struct renewal_state *state = tevent_req_data(req, struct renewal_state);
> +
> + DEBUG(SSSDBG_CRIT_FAILURE, "Timeout reached for AD renewal
child.\n");
> + child_handler_destroy(state->child_ctx);
> + state->child_ctx = NULL;
> + state->child_status = ETIMEDOUT;
> + tevent_req_error(req, ERR_RENEWAL_CHILD);
> +}
> +
> +static errno_t
> +ad_machine_account_password_renewal_recv(struct tevent_req *req)
> +{
> +
> + TEVENT_REQ_RETURN_ON_ERROR(req);
> +
> + return EOK;
> +}
> +
> +errno_t ad_machine_account_password_renewal_init(struct be_ctx *be_ctx,
> + struct dp_option *opts)
> +{
> + int ret;
> + struct renewal_data *renewal_data;
> + int lifetime;
> + const char *dummy;
> + char **opt_list;
> + int opt_list_size;
> + char *endptr;
> +
> + lifetime = dp_opt_get_int(opts, AD_MAXIMUM_MACHINE_ACCOUNT_PASSWORD_AGE);
> +
> + if (lifetime == 0) {
> + DEBUG(SSSDBG_CONF_SETTINGS, "Automatic machine account renewal
disabled.\n");
> + return EOK;
> + }
> +
> + if (lifetime < 0) {
> + DEBUG(SSSDBG_CRIT_FAILURE,
> + "Illegal value [%d] for password lifetime.\n", lifetime);
> + return EINVAL;
> + }
> +
> + renewal_data = talloc(be_ctx, struct renewal_data);
> + if (renewal_data == NULL) {
> + DEBUG(SSSDBG_OP_FAILURE, "talloc failed.\n");
> + return ENOMEM;
> + }
> +
> + dummy = dp_opt_get_cstring(opts, AD_MACHINE_ACCOUNT_PASSWORD_RENEWAL_OPTS);
> + ret = split_on_separator(renewal_data, dummy, ':', true, false,
> + &opt_list, &opt_list_size);
> + if (ret != EOK) {
> + DEBUG(SSSDBG_OP_FAILURE, "split_on_separator failed.\n");
> + goto done;
> + }
> +
> + if (opt_list_size != 2) {
> + DEBUG(SSSDBG_CRIT_FAILURE, "Wrong number of renewal
options.\n");
> + ret = EINVAL;
> + goto done;
> + }
> +
> + errno = 0;
> + renewal_data->period = strtouint32(opt_list[0], &endptr, 10);
> + if (errno != 0 || *endptr != '\0' || opt_list[0] == endptr) {
> + DEBUG(SSSDBG_CRIT_FAILURE, "Unable to parse first renewal
option.\n");
> + ret = EINVAL;
> + goto done;
> + }
> +
> + errno = 0;
> + renewal_data->initial_delay = strtouint32(opt_list[1], &endptr, 10);
> + if (errno != 0 || *endptr != '\0' || opt_list[0] == endptr) {
> + DEBUG(SSSDBG_CRIT_FAILURE, "Unable to parse second renewal
option.\n");
> + ret = EINVAL;
> + goto done;
> + }
> +
> + renewal_data->pw_lifetime_in_days = lifetime;
> + renewal_data->ad_domain = talloc_strdup(renewal_data,
> + dp_opt_get_string(opts, AD_DOMAIN));
> +
> + renewal_data->ad_hostname = talloc_strdup(renewal_data,
> + dp_opt_get_string(opts,
> + AD_HOSTNAME));
> + renewal_data->ad_keytab = talloc_strdup(renewal_data,
> + dp_opt_get_string(opts, AD_KEYTAB));
I don't think we need to dup the options either -- looks like we won't
modify them later.
I changed to code a bit an removed the members from the struct.
> + if (renewal_data->ad_domain == NULL || renewal_data->ad_hostname == NULL
> + || renewal_data->ad_keytab == NULL) {
> + DEBUG(SSSDBG_CRIT_FAILURE, "Missing AD domain or hostname.\n");
> + ret = EINVAL;
> + goto done;
> + }
> +
> + ret = get_adcli_extra_args(renewal_data);
> + if (ret != EOK) {
> + DEBUG(SSSDBG_OP_FAILURE, "get_adcli_extra_args failed.\n");
> + goto done;
> + }
Additionally I found that I use the wrong time for the child timeout.
The new second patch adds the new call be_ptask_get_timeout() to get the
timeout of the running task.
Otherwise the code looks good. I'm going to run some tests now..
Thank you for the review. New version attached.
bye,
Sumit