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..
+ 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",
+
+ 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.
+ 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;
+ }
Otherwise the code looks good. I'm going to run some tests now..