Bug 93282 - Conditionally 'update' computer account password
Summary: Conditionally 'update' computer account password
Status: RESOLVED FIXED
Alias: None
Product: realmd
Classification: Unclassified
Component: adcli (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Stef Walter
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-12-07 09:03 UTC by Stef Walter
Modified: 2015-12-11 10:32 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Check host password lifetime (9.11 KB, patch)
2015-12-08 21:13 UTC, Sumit Bose
Details | Splinter Review
Check host password lifetime - v2 (10.85 KB, patch)
2015-12-11 10:15 UTC, Sumit Bose
Details | Splinter Review

Description Stef Walter 2015-12-07 09:03:27 UTC
We should have an option to conditionally update the computer account password with 'adcli update'. Doing this too often can run afoul of password change frequency policy. 

Sumit has done some work here, but it needs refactoring:

https://bugs.freedesktop.org/attachment.cgi?id=120347

 * We should split out the time parser, and add tests for it.
 * Fix the TODOs
Comment 1 Sumit Bose 2015-12-08 21:13:59 UTC
Created attachment 120420 [details] [review]
Check host password lifetime
Comment 2 Sumit Bose 2015-12-08 21:15:19 UTC
Please have a look at the attached patch, it moves the lifetime check in a new function, adds a test and fixes the TODOs.
Comment 3 Stef Walter 2015-12-08 22:07:23 UTC
Comment on attachment 120420 [details] [review]
Check host password lifetime

Review of attachment 120420 [details] [review]:
-----------------------------------------------------------------

Overall this looks good. Just a few comments.

::: doc/adcli.xml
@@ +364,4 @@
>  			computer account to this Kerberos principal.</para></listitem>
>  		</varlistentry>
>  		<varlistentry>
> +			<term><option>-host-pw-lifetime=<parameter>lifetime</parameter></option></term>

How about we use an argument --minimum-password-lifetime or --computer-password-lifetime?

In any case I think using 'computer' here would be better than 'host' because that's the term that active directory uses.

::: library/adenroll.c
@@ +98,5 @@
>  	krb5_principal *keytab_principals;
>  	krb5_enctype *keytab_enctypes;
>  	int keytab_enctypes_explicit;
> +	unsigned int host_pw_lifetime;
> +	int host_pw_lifetime_explicit;

How about we call this computer_password_lifetime to be consistent.

@@ +1643,5 @@
>  
> +	ldap = adcli_conn_get_ldap_connection (enroll->conn);
> +	assert (ldap != NULL);
> +
> +	value = _adcli_ldap_parse_value (ldap,

I think 'value' leaks.

@@ +1648,5 @@
> +	                                 enroll->computer_attributes,
> +	                                 "pwdLastSet");
> +
> +	if (check_nt_time_string_lifetime(value,
> +	                                  adcli_enroll_get_host_pw_lifetime(enroll))) {

These need spaces after the function names.

@@ +2263,5 @@
> +}
> +
> +void
> +adcli_enroll_set_host_pw_lifetime (adcli_enroll *enroll,
> +                                   const char *value)

I would suggest accepting an integer value to this function, and doing the parse from a string in the caller, so we can print a proper error message.

@@ +2270,5 @@
> +
> +	return_if_fail (enroll != NULL);
> +	errno = 0;
> +	enroll->host_pw_lifetime = strtoul(value, &endptr, 10);
> +	return_if_fail(errno == 0 && *endptr == '\0' && endptr != value);

This is not an unexpected error. It can happen due to invalid input. But in any case lets do the parsing in the caller.

::: library/adutil.c
@@ +390,5 @@
>  
> +#define AD_TO_UNIX_TIME_CONST 11644473600LL
> +
> +bool
> +check_nt_time_string_lifetime (const char *nt_time_string, unsigned int lifetime)

Arguments should be on different lines.

Lets also give this function an _adcli_ prefix?

@@ +398,5 @@
> +	char *endptr;
> +	time_t now;
> +
> +	if (nt_time_string == NULL) {
> +		_adcli_err("Missing NT time string, assuming it is expired");

Needs a space after function call.

@@ +401,5 @@
> +	if (nt_time_string == NULL) {
> +		_adcli_err("Missing NT time string, assuming it is expired");
> +		return false;
> +	}
> +	now = time(NULL);

Needs a space after function call.

@@ +407,5 @@
> +	nt_now = (now + AD_TO_UNIX_TIME_CONST) * 1000 * 1000 * 10;
> +	errno = 0;
> +	pwd_last_set = strtoull (nt_time_string, &endptr, 10);
> +	if (errno != 0 || *endptr != '\0' || endptr == nt_time_string) {
> +		_adcli_err("Failed to convert NT time string, assuming it is expired");

Needs a space after function call.

@@ +468,5 @@
>  	assert_num_eq (len, 3);
>  }
>  
> +static void
> +test_check_nt_time_string_lifetime(void)

All the function names here need spaces after them.

::: tools/computer.c
@@ +409,4 @@
>  		{ "os-version", required_argument, NULL, opt_os_version },
>  		{ "os-service-pack", optional_argument, NULL, opt_os_service_pack },
>  		{ "user-principal", optional_argument, NULL, opt_user_principal },
> +		{ "host-pw-lifetime", optional_argument, NULL, opt_host_pw_lifetime },

How about we use an argument --minimum-password-lifetime or --computer-password-lifetime?

In any case I think using 'computer' here would be better than 'host' because that's the term that active directory uses.
Comment 4 Sumit Bose 2015-12-11 10:15:20 UTC
Created attachment 120462 [details] [review]
Check host password lifetime - v2

Thank you for the review, please find attached an updated version of the patch.

Btw, I added some rules to my vim configuration to spot missing spaces before (, can you share your editor/indent settings to make it easier to meet the coding style?
Comment 5 Stef Walter 2015-12-11 10:30:27 UTC
Comment on attachment 120462 [details] [review]
Check host password lifetime - v2

Patch looks good. I won't be able to test this properly here (seeing that it needs multiple days of testing) but I'll trust that you have and will test this.
Comment 6 Stef Walter 2015-12-11 10:30:52 UTC
Attachment 120420 [details] pushed as de8c7d6 - Check host password lifetime
Comment 7 Stef Walter 2015-12-11 10:32:56 UTC
Hmmm, this is actually the patch that was pushed:

https://bugs.freedesktop.org/attachment.cgi?id=120462

I think 'git bz' got confused by the Attachment title.

(In reply to Sumit Bose from comment #4)
> Btw, I added some rules to my vim configuration to spot missing spaces
> before (, can you share your editor/indent settings to make it easier to
> meet the coding style?

I think this is it:

https://github.com/stefwalter/vimrc/blob/master/gvimrc#L32


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.